STR: 1) Install Personas plus 1.6 and Google toolbar (https://addons.mozilla.org/en-US/statistics/addon/6249) on Windows (7 and vista have shown this bug) 2) Restart Firefox 3) Firefox loads, but freezes. A slow script warning is shown: http://grab.by/64DK Expected results: Firefox loads both add-ons successfully. We need to know if this is a problem with Personas or Google toolbar and if the latter what they are doing wrong.

got the same warning script warning when I installed both in ubuntu. Also, it threw me one with this path: /path/to/profile/firebug@software.joehewitt.com/components/firebug-service.js:2353

quickfix: with both addons installed (Personas Plus and the Google Toolbar) Uninstall Personas Pluss Restart Firefox Reinstall Personas Plus Restart Firefox Testing in both ubuntu and vista this seems to work, as the slow script warning goes away and there aren't any obvious performance issues.

Also happens in Mac. I'm getting this error when Firefox launches: [GTB-bootstrap] exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: chrome://google-toolbar-lib/content/toolbar.js :: GTB_Uninstaller :: line 1922" data: no], stack: undefined

cc'ing Myk, I've also emailed our Google contact. Myk: can you lend a hand figuring out the problem?

The error mentioned in comment #3 only shows up the first time Firefox runs after Personas is installed; subsequent runs do not cause the error to appear. We tried checking out Google Toolbar’s toolbar.js file, but the code is minified. According to our research, this error might occur because Personas is still in the middle of component registration when Google Toolbar tries to get a hold of the component. But we’re not exactly sure. When Firefox gets stuck, the method "stripPrefix" from nsExtensionManager.js is being called non-stop.

For reference, this is reproducible with Google Toolbar 7.1.20100723W. Personas 1.5.3 does not seem to have this problem. Someone please sandbox version 1.6 on AMO. A blocklist entry may also be needed here. By the way, if you have Flagfox installed it will also throw up an exception popup in response to the broken extension manager: TypeError: ExtensionManager.getItemForID(id) is null I have more than a few confused users on the Flagfox forums reporting this.

Regarding Flagfox, I tried it alongside Personas 1.6 and everything seems to work correctly to me. (Only Personas 1.6 and Flagfox installed, in Mac.)

Yeah, I can only reproduce the error when Google Toolbar is installed. I have one guy noting on our forums that he doesn't have the Google Toolbar but does have Personas and is getting problems. I don't know what else is going on there.

The errors with Flagfox don't seem to occur if you've uninstalled then reinstalled personas plus 1.6.

(In reply to comment #12) > The errors with Flagfox don't seem to occur if you've uninstalled then > reinstalled personas plus 1.6. In fact, I can't repro this at all anymore on win7. Something changes permanently after version 1.6.

Yeah, see comment 2 which says just a reinstall of Personas worked. Ryan, is it that you can't reproduce the initial stall or is the extension manager also not stalling on load?

(In reply to comment #15) > Yeah, see comment 2 which says just a reinstall of Personas worked. > > Ryan, is it that you can't reproduce the initial stall or is the extension > manager also not stalling on load? Ah, now I was able to repro. I kept the 'make google my default search engine' checkbox selected on the google toolbar download page. Before, nothing was stalling.

(In reply to comment #16) > Ah, now I was able to repro. I kept the 'make google my default search engine' > checkbox selected on the google toolbar download page. > > Before, nothing was stalling. And after killing Firefox and starting up again, everything works (no initial stall, add-ons manager opens fine)

It usually takes a restart _after_ the install's restart to get the problems.

Our dev team has tested Forecastfox, Tab Mix Plus, Firebug, and Flagfox with Personas Plus and not found any incompatibilities on a clean profile. http://people.mozilla.org/~rdoherty/googletoolbarerror.mov

(In reply to comment #19) > Our dev team has tested Forecastfox, Tab Mix Plus, Firebug, and Flagfox with > Personas Plus and not found any incompatibilities on a clean profile. > > http://people.mozilla.org/~rdoherty/googletoolbarerror.mov Yes, it is as soon as Google Toolbar is installed (in conjunction with Personas 1.6) that all other add-ons begin to fail. (See the end of video)

(In reply to comment #18) > It usually takes a restart _after_ the install's restart to get the problems. I've tested a few times and once you kill Firefox the first time the problem goes away. I can open the add-ons manager and it works.

Changing to all OS's, I was able to repro on OSX 10.6.4

Under WinXP I was getting stalls going into the extension manager each time.

Getting same error in Win7 Enterprise x64. Noticed after latest patch pushed this week for personas. Disabled personas and Google toolbar resumes normal operating conditions. Also get script error when Mozilla locks up.

(In reply to comment #2) > quickfix: > with both addons installed (Personas Plus and the Google Toolbar) > > Uninstall Personas Pluss > Restart Firefox > Reinstall Personas Plus > Restart Firefox > > Testing in both ubuntu and vista this seems to work, as the slow script warning > goes away and there aren't any obvious performance issues. This fixed my issue in Windows 7. After I uninstalled personas, restarted firefox, disabled google toolbar, then reinstalled personas. restared firefox, enabled google toolbar and all are working fine now.

Has anyone found any evidence as to where the actual code fault is? It could be in Personas Plus, Google Toolbar, in Firefox or Toolkit, or all of the above.

We're getting more reports of a slow script dialog from nsExtensionManager.js as well with some Brand Thunder addons. These weren't happening before Firefox 3.6.7. We're checking to see what other addons those people have installed.

I think we would all like to have this resolved by a nice easily pushed update that has yet to come, but thus far we're still seeing problems from this and some form of blocklisting is starting to look like a better option than I had hoped. 1.1 million people are potentially affected in some way by this update snafu. See bug 590978. The nice video in comment 19-20 demonstrates the problem in graphic detail, but you can install the same things in the opposite order and get the same problems, thus I still don't know where the actual fault is here. Is there a helpful Personas Plus update in the works? Is there a helpful Google Toolbar update in the works? Can someone from Mozilla Labs please give an update?

Google Toolbar has nothing to do with it. I installed one of our brand thunder addons, then installed Personas Plus, then rebooted and our addon is broke and extension manager won't come up.

Thanks. Other addons choke as well? (Flagfox, Forecastfox, etc.)

Everything chokes. The problem is that Personas 1.6 overrides extension manager with its own nsPersonasExtensionManager.js. So any addon that uses the extension manager is failing (I haven't tracked down what they are doing wrong) The reason my addons die is because actually getting the extension manager service is failing. Removing that component and then removing xpti.dat and compreg.dat fixes everything. (I had to remove xpti.dat and compreg.dat or Firefox wouldn't even start). The sad part is that there was absolutely no need for the Personas team to override the extensionmanager code to get coexistence with themes working. I was able to get it working with a couple overrides of some LightweightThemeManager functions. This was just a really bad idea.

Thank you very much for the thorough investigation.

Google has a fix for their add-on and has released it. We'll do further testing with it and Personas Plus 1.6 to verify the fix works before releasing the update. We'll also re-check compatibility with the other add-ons listed.

rdoherty: Please remove that code entirely. It was completely unnecessary in order to allow Personas and themes to coexist.

(In reply to comment #37) > rdoherty: Please remove that code entirely. > > It was completely unnecessary in order to allow Personas and themes to coexist. Not going to happen in the near future. We'll do further testing on our end regarding the add-ons listed to have problems.

(In reply to comment #37) > rdoherty: Please remove that code entirely. > > It was completely unnecessary in order to allow Personas and themes to coexist. Can you specify why it is not necessary? According to Dao and Myk, it is necessary to override the nsExtensionManager component in order to allow Personas to be applied over Lightweight Themes: === Dao: It's the extension manager and not the LTM that prevents personas from being used with third-party themes. See http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#2294 Myk: That's good news, since the extension manager is an XPCOM component, which means it's straightforward to override it with a modified one shipped with Personas Plus. The only change you'll have to make to ship it with the addon (besides the Personas-specific customizations) is replace the preprocessing instructions that modify the script based on the OS on which Firefox is being built with JavaScript conditionals that do the same thing at runtime. I've done that before, and it's fairly easy to do (and works as expected). === "I was able to get it working with a couple overrides of some LightweightThemeManager functions" Can you specify how you did it? This will help us address the issue. Thanks

> "I was able to get it working with a couple overrides of some LightweightThemeManager functions" > Can you specify how you did it? This will help us address the issue. Thanks Sure. I reimplemented setCurrentTheme. And I did this: /* BEGIN - HACK */ var selectedSkin; if (_allprefs.prefHasUserValue("general.skins.selectedSkin")) { selectedSkin = _allprefs.getCharPref("general.skins.selectedSkin"); _allprefs.clearUserPref("general.skins.selectedSkin"); } /* END - HACK */ _observerService.notifyObservers(cancel, "lightweight-theme-change-requested", JSON.stringify(aData)); /* BEGIN HACK */ if (selectedSkin) { _allprefs.setCharPref("general.skins.selectedSkin", selectedSkin); } /* END HACK */ I realize it was hacky, but it was much safer than messing around with the extension manager. I also had to do it in preview: let cancel = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool); cancel.data = false; _observerService.notifyObservers(cancel, "lightweight-theme-preview-requested", JSON.stringify(aData)); /* Ignore cancel.data so preview works on non classic skins */ _observerService.notifyObservers(null, "lightweight-theme-styling-update", JSON.stringify(aData)); Clearly you are much safer messing around with LightweightThemeManager then with the extension manager (which has the potential to break a lot of things) >> rdoherty: Please remove that code entirely. >Not going to happen in the near future. We'll do further testing on our end >regarding the add-ons listed to have problems. Then can you please explain what the bug was in Personas and how you are going to prevent it from ever happening again? What is the fix?

Our information from Google: --- It seems that we were pulling the extension manager instance into our code "too" early. During the XPCOM component (extension) initialization the getService call on the extension manager component failed with an exception, and this made our initialization fail. It appears to be a race condition, so it may be hard to reproduce it in an isolated setting. --- I've tested the update to Google Toolbar with Personas Plus 1.6 and it works for me. I've also tested ForecastFox, Firebug, Tab Mix Plus and Personas Interactive and all work with a blank profile and Personas Plus 1.6.

So, in response to the who's at fault question, the answer: both. Great. Glad they've fixed their problem. We can keep this bug open to fix the problems Michael is reporting in Personas Plus 1.6.

You do not even (need to) override anything in the original extension manager, you just "redirect" some topics. You can do this without replacing nsExtensionManager with your own implementation. * Don't override the original implementation; register your own component * Unregister the nsEM lwt observers * Unregister the nsEM profile-after-change category manager entry for profile-after-change * Register your own observers / category manager entry. * Make sure to pass through the profile-after-change to the nsEM.

Overriding the extension manager like this will cause all kinds of problem (as demonstrated by other extensions). Just don't do it. ;) (Same holds true for a lot of other core services). Furthermore the current code is quite buggy (though I didn't check if the bugs are actually triggered), e.g. QI will always drop the nsEM reference and get a new one (refcount hazard), observers might be called more than once; to name a few. So either review and take my patch or do another thorough review and bugfixing of the existing code.

If there's some field to mark this as blocking the next Personas Plus update, please do so. (I see a 1.7 in the milestone list, but I don't know if you'd rather just go to 1.6.1 or what your practices are in this component) As per recent discussion in bug 590978, Google is updating their toolbar rather quickly so hopefully the main interaction problem will be largely resolved soon, however I would really prefer it if 1.6 was not re-pushed as-is and a fix here get out asap.

Comment on attachment 473640 [details] [diff] [review] patch v1, Do not override nsExtensionManager, just "redirect" topics. >+// If defineLazyServiceGetter is not present we won't load anyway, as this is >+// 3.6+ only >+if ('defineLazyServiceGetter' in XPCOMUtils) { >+ XPCOMUtils.defineLazyServiceGetter( >+ this, "gRDF", >+ "@mozilla.org/rdf/rdf-service;1", "nsIRDFService" >+ ); >+ XPCOMUtils.defineLazyServiceGetter( >+ this, "gExtMan", >+ "@mozilla.org/extensions/manager;1", "nsIExtensionManager" >+ ); >+ XPCOMUtils.defineLazyServiceGetter( >+ this, "gIoServ", >+ "@mozilla.org/network/io-service;1", "nsIIOService" >+ ); >+ XPCOMUtils.defineLazyServiceGetter( >+ this, "gAppStartup", >+ "@mozilla.org/toolkit/app-startup;1", "nsIAppStartup" >+ ); >+} You have a comment explicitly stating why the following line is useless. Why not just axe the check then?

(In reply to comment #46) > Comment on attachment 473640 [details] [diff] [review] > patch v1, Do not override nsExtensionManager, just "redirect" topics. > > >+// If defineLazyServiceGetter is not present we won't load anyway, as this is > >+// 3.6+ only > >+if ('defineLazyServiceGetter' in XPCOMUtils) { ... > >+} > > You have a comment explicitly stating why the following line is useless. Why > not just axe the check then? It's not useless, it will prevent the code from running in FX<3.6 and producing an exception because defineLazyServiceGetter is not defined.

Sorry guys, I'm not updating my addons to fix this. We have 60 or 70 addons in the field that are broken because of this. What Personas is doing is not acceptable.

(In reply to comment #48) > Sorry guys, I'm not updating my addons to fix this. We have 60 or 70 addons in > the field that are broken because of this. What are the addons? We want to test this also. Thanks!

Every AMO hosted Addon by brand thunder: https://addons.mozilla.org/en-US/firefox/user/37347/ As well as all of the self hosted addons before July 1st. They are all using the same code internally, so they all show the same problem. The issue would be the same as reported for google toolbar. Doing this: const gExtensionManager = Cc["@mozilla.org/extensions/manager;1"].getService(Ci.nsIExtensionManager); During startup of my component. Obviously lazy getters would be the solution in theory, but this stuff is all designed to work on Firefox 3.0 and it all worked before Personas 1.6.

(In reply to comment #50) > Every AMO hosted Addon by brand thunder: > > https://addons.mozilla.org/en-US/firefox/user/37347/ Confirmed, I tested 2 and see the same slow script warning.

In case you haven't seen these: http://support.mozilla.com/en-US/questions/749357 (48 people have this problem) https://support.mozilla.com/en-US/questions/749183 (132 people have this problem) http://support.mozilla.com/en-US/questions/749314 (574 people have this problem) Other reports: http://support.mozilla.com/en-US/search?w=2&q=nsExtensionManager.js

Build with my patch v1 applied for everybody to test. (Regular users should probably wait for an official release; this one isn't official!) (In reply to comment #50) > Every AMO hosted Addon by brand thunder: > > https://addons.mozilla.org/en-US/firefox/user/37347/ > > As well as all of the self hosted addons before July 1st. > > They are all using the same code internally, so they all show the same problem. I just tested my patch with https://addons.mozilla.org/en-US/firefox/addon/5043/ . No slow script warning. mkaply, can you verify this? And, of course, that lwt for non-default themes still works? (Might want to set lightweightThemes.forceSkinning = true)

Unfortunately when the browser is in this state, you can't install XPIs.I will test with a new profile. I'm sorry guys, but I'm not satisfied with these solutions. Everyone that is broken will stay broken. You're going to have to think bigger than this. I don't know how this problem can be worked around via an extension since the extension manager is already broke at this point.

(In reply to comment #53) > Created attachment 473709 [details] > Unofficial build with patch v1 applied for testing > > Build with my patch v1 applied for everybody to test. > (Regular users should probably wait for an official release; this one isn't > official!) > > (In reply to comment #50) > > Every AMO hosted Addon by brand thunder: > > > > https://addons.mozilla.org/en-US/firefox/user/37347/ > > > > As well as all of the self hosted addons before July 1st. > > > > They are all using the same code internally, so they all show the same problem. > > I just tested my patch with > https://addons.mozilla.org/en-US/firefox/addon/5043/ . No slow script warning. > > mkaply, can you verify this? > And, of course, that lwt for non-default themes still works? > (Might want to set lightweightThemes.forceSkinning = true) I can confirm that it works correctly (on a stable Firefox profile, that is). The code changes look good to me too.

(In reply to comment #54) > Unfortunately when the browser is in this state, you can't install XPIs.I will > test with a new profile. Firefox safe mode can be used to disable add-ons.

I can confirm this fix works for me on a stable profile as well.

> Firefox safe mode can be used to disable add-ons. A typical user has no idea what add-on caused the problem and no idea how to start safe mode. In our case, our add-on displays wrong, so users thing it is our add-on causing the problem. They go to open add-ons and it doesn't work. You guys are utterly failing to see the big picture here. You have a potentially a million people stranded on Personas 1.6.

We're taking a look at the supplied fix right now and will make a decision soon as to our next steps.

I'll also point out that the Google Toolbar fix is useless as well since people will not actually receive the Google Toolbar update because their extension manager is broke. I opened bug 594940 to address the issue that core Firefox code will probably need to be changed to work around this problem. And a note to the AMO team - addons that replace the extension manager should probably not be allowed on AMO. There's too much risk for breakage.

Comment on attachment 473640 [details] [diff] [review] patch v1, Do not override nsExtensionManager, just "redirect" topics. This looks fine, r=myk.

I did a basic sanity test of a new build with the fix, no compatibility issues found.

(In reply to comment #60) > I'll also point out that the Google Toolbar fix is useless as well since people > will not actually receive the Google Toolbar update because their extension > manager is broke. Bug 590978 comment 19 said that Google was getting this thing updated (possibly through their own voodoo). If that's not the case, then yeah, blocklist time. Ryan, which is it?

(In reply to comment #63) > Bug 590978 comment 19 said that Google was getting this thing updated (possibly > through their own voodoo). If that's not the case, then yeah, blocklist time. > Ryan, which is it? Google is updating their add-on with a fix that makes their add-on work, but there's no way to track if the users being updated also have Personas Plus installed. Blocklist discussion is going on in bug 590978.

(In reply to comment #47) > (In reply to comment #46) > > It's not useless, it will prevent the code from running in FX<3.6 and producing > an exception because defineLazyServiceGetter is not defined. The comment implied not loading at all <3.6. Apparently you still do support it, which just means the comment is unclear as to what "this" is going to fail. Nevermind then; no need to nit-pick comments if that's the case.

I haven't seen it mentioned here so it might be useful to say that I think I can see what the problem probably is here. This section of code attempts to create wrappers around all of the extension manager's properties and functions such that retrieving them will call through to the real versions on the extension manager: function inheritCurrentInterface(self, base) { function defineGetter(prop) { self.__defineGetter__(prop, function() base[prop]); } for(let prop in base) { if(typeof self[prop] === 'undefined') if(typeof base[prop] === 'function') { (function(prop) { self[prop] = function() { return base[prop].apply(base,arguments); }; })(prop); } else defineGetter(prop); } } The problem is the line "if(typeof self[prop] === 'undefined')". This doesn't just test if a property is already defined on self it actually retrieves it too. In the case of getters this means that the getter function is executed on the extension manager. All that needs to happen for these getters to be executed is two pieces of code have to attempt to retrieve the extension manager service and QI it to the same interface (the first time just creates the wrappers for the getters, the second executes them during this bogus test). Basically with this in place code in the extension manager is executed that normally wouldn't be just by retrieving the service. This could potentially cause infinite recursion since the getter code could call into something that then tries to get the EM service, but I think what is actually happening here is that some extensions are retrieving the EM before profile-after-change. This would cause the datasource getter to be called which would attempt to load extensions.rdf, but since this is before the profile is known that attempt would fail and unfortunately fail in a way that meant the datasource would never be accessible. I've verified this locally with no extensions installed and it matches up with the errors I saw when testing extension update and app update and makes opening the add-ons manager hang the browser with periodic slow script warnings. At least this specific problem is something that we can fix in the extension manager side very easily so I've filed bug 595092 to do that.

RC here, tested and uploaded to amo: http://people.mozilla.org/~rdoherty/personas-1.6.1rc1.xpi

This is fixed, right? There's a Personas 1.6.1 available, the old one is blocklisted, and a new client (3.6.10) was shipped with a built-in blocklist. Anything else to do here?

(In reply to comment #69) > This is fixed, right? There's a Personas 1.6.1 available, the old one is > blocklisted, and a new client (3.6.10) was shipped with a built-in blocklist. > Anything else to do here? In this bug? I guess not.