Rob Arnold [:robarnold] Reporter
	Description • 14 years ago

I updated from the May 15th build to the May 17th build and found that NoScript had been uninstalled without any notice. I reinstalled it from AMO and restarted. It did not appear in the list of installed extensions but it was clearly installed as the icon was sitting in my status bar. Switching away from the extensions item (to say, themes) and then back somehow made it show up. It will continue to show up even if you close and reopen the addons manager until "some time later" (a GC?).

	Dave Townsend [:mossop] Assignee
	Comment 1 • 14 years ago

Turn on extensions.logging.enabled and see if anything shows up in the error console after you've opened the add-ons manager and NoScript hasn't shown up in the list.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 2 • 14 years ago

I can see the same after a restart. NoScript doesn't appear. I will check the logs now.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 3 • 14 years ago

Ok, so I can't see that NoScript gets deinstalled but it's not visible in the list of extensions for the first time.

Steps:
1. Install NoScript from AMO
2. Restart Firefox
3. Open the Add-ons Manager and go to the Extension pane
4. Select the Themes pane
5. Select the Extensions pane

After step 2 NoScript is installed and shows up as an icon at the bottom right corner. Checking step 3 doesn't list that extension. But it's shown again after step 5.

I wonder if I see another issue with the initial loading of add-ons or if it is the same root cause.

	Antti Tervasmäki
	Comment 4 • 14 years ago

May be related: 
When I open Add-ons Manager it never shows all my extensions, only some: 
Error Console: Warning: WARN addons.manager: Exception calling callback: TypeError: aAddon.selectedLocale is undefined) 

After selecting Themes and again Extensions, I see all of them.

	Daniel Holbert [:dholbert]
	Comment 6 • 14 years ago

Note that this bug makes it appear impossible to uninstall any extensions that are missing from the list.  (This almost led me to trash my main browsing profile & start over from scratch, due to not being able to uninstall an extension that was causing me problems, until I discovered how to make it show up in the list.)

Platform --> All/All, since this isn't win7-specific.  I'm using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100519 Minefield/3.7a5pre

	Tony Mechelynck [:tonymec]
	Comment 7 • 14 years ago

I get this every time for the Debug & QA UI in SeaMonkey 2.1a2pre (i.e. the current trunk nightlies). This extension is shipped built-in with nightly builds, but when I open the addons-manager it is not listed the first time. Going to Themes and back to Extensions makes it appear. This is repeatable, at least for me:

1. Install the latest SeaMonkey trunk nightly.
2. Start it with a virgin profile.
3. Open the addons manager.
--- Only Chatzilla, DOM Inspector and JavaScript Debugger are shown.
4. Select Themes in the sidebar.
5. Select Extensions in the sidebar.
--- Debug & QA UI has been added.

If it makes any difference, I'm on Linux; and the "Debug" and "QA" menus provided by this extension are present in the menubar from the start, showing that even if the addons manager doesn't list it, it is there.

	Dave Townsend [:mossop] Assignee
	Comment 8 • 14 years ago

Interestingly a regression from bug 564092

	Dave Townsend [:mossop] Assignee
	Comment 9 • 14 years ago

This is caused by a race condition in the API when a request for add-ons is made while another request is still being processed.

The problem arises because we cache instances of add-ons when we get them from the DB and then go on to fetch their metadata which involved more async DB operations. When a second request comes along for the same add-on it gets it from the cache and then proceeds to request its metadata even though that is already being filled out by a different async task. Re-entrancy into that part of the code isn't handled well and so some properties get set to bad values. We also can't just bail out and not fetch the metadata for the second retrieval since then we hand back a partially filled instance, instead we must wait for the metadata to be fully retrieved.

This patch adds a queue of callbacks that are waiting for the add-on instance. The first attempt to get an add-on starts the metadata retrieval, subsequent attempts just add their callback to the queue and bail early. It also adds a helper object that does the right thing for all the async retrievals.

	Robert Strong (they/them - no direct email)
	Comment 10 • 14 years ago

Comment on attachment 447549 [details] [diff] [review]
patch rev 1

>@@ -2876,62 +2932,48 @@ var XPIDatabase = {
>    * @param  aCallback
>    *         A callback to pass the DBAddonInternal to
>    */
>   getAddonInLocation: function XPIDB_getAddonInLocation(aId, aLocation, aCallback) {
>     let stmt = this.getStatement("getAddonInLocation");
> 
>     stmt.params.id = aId;
>     stmt.params.location = aLocation;
>-    stmt.executeAsync({
>-      addon: null,
>-
>-      handleResult: function(aResults) {
>-        this.addon = XPIDatabase.makeAddonFromRowAsync(aResults.getNextRow());
>-      },
>-
>-      handleError: asyncErrorLogger,
>-
>-      handleCompletion: function(aReason) {
>-        if (this.addon)
>-          XPIDatabase.fetchAddonMetadata(this.addon, aCallback);
>-        else
>-          aCallback(null);
>-      }
>-    });
>+    stmt.executeAsync(new AsyncAddonListCallback(function(aAddons) {
>+      if (aAddons.length == 0) {
>+        aCallback(null);
>+        return;
>+      }
>+      if (aAddons.length > 1)
>+        WARN("Multiple addons with ID " + aId + " found in location " + aLocation);
Is this just defensive or is there a way for a location to have more than one add-on with the same ID? If this does occur shouldn't some cleanup should occur? Here and in getVisibleAddonForID.

	Dave Townsend [:mossop] Assignee
	Comment 11 • 14 years ago

(In reply to comment #10)
> (From update of attachment 447549 [details] [diff] [review])
> >@@ -2876,62 +2932,48 @@ var XPIDatabase = {
> >    * @param  aCallback
> >    *         A callback to pass the DBAddonInternal to
> >    */
> >   getAddonInLocation: function XPIDB_getAddonInLocation(aId, aLocation, aCallback) {
> >     let stmt = this.getStatement("getAddonInLocation");
> > 
> >     stmt.params.id = aId;
> >     stmt.params.location = aLocation;
> >-    stmt.executeAsync({
> >-      addon: null,
> >-
> >-      handleResult: function(aResults) {
> >-        this.addon = XPIDatabase.makeAddonFromRowAsync(aResults.getNextRow());
> >-      },
> >-
> >-      handleError: asyncErrorLogger,
> >-
> >-      handleCompletion: function(aReason) {
> >-        if (this.addon)
> >-          XPIDatabase.fetchAddonMetadata(this.addon, aCallback);
> >-        else
> >-          aCallback(null);
> >-      }
> >-    });
> >+    stmt.executeAsync(new AsyncAddonListCallback(function(aAddons) {
> >+      if (aAddons.length == 0) {
> >+        aCallback(null);
> >+        return;
> >+      }
> >+      if (aAddons.length > 1)
> >+        WARN("Multiple addons with ID " + aId + " found in location " + aLocation);
> Is this just defensive or is there a way for a location to have more than one
> add-on with the same ID? If this does occur shouldn't some cleanup should
> occur? Here and in getVisibleAddonForID.

Purely defensive. I don't believe it can occur, but I'd rather know about it if it does.

	Robert Strong (they/them - no direct email)
	Comment 12 • 14 years ago

Comment on attachment 447549 [details] [diff] [review]
patch rev 1

Please add a comment in the two places you have the defensive measure that this should never happen... I kind of think they should use ERROR as well.

	Volkmar Kostka
	Comment 13 • 14 years ago

Weird. On my first start to day my old toolbar customizations (and all custom buttons) were back and the WebDeveloperToolbar was there and working. Now after updating and restarting they are gone again.

Can the race condition also affect toolbars and custom buttons?

	Dave Townsend [:mossop] Assignee
	Comment 14 • 14 years ago

(In reply to comment #13)
> Weird. On my first start to day my old toolbar customizations (and all custom
> buttons) were back and the WebDeveloperToolbar was there and working. Now after
> updating and restarting they are gone again.
> 
> Can the race condition also affect toolbars and custom buttons?

No, it is possible you are seeing something like bug 562930 though.

	Dave Townsend [:mossop] Assignee
	Comment 15 • 14 years ago

Landed with those changes: http://hg.mozilla.org/mozilla-central/rev/03bd98ae2bf3

The automated test in this verifies that sequenced use of the API doesn't break, however since the problems here are caused by timing conditions it isn't really possible to fully test this automatically. I'm not really sure it is possible to do it manually either, we're lucky that this turned out to be fairly reproducable so I could spot what was going on.

	Tony Mechelynck [:tonymec]
	Comment 16 • 14 years ago

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100527 SeaMonkey/2.1a2pre - Build ID: 20100527204731

I VERIFY that I don't see the bug in this tinderbox-build, dated some two hours after comment #15. I see it neither in a fresh profile as in comment #7, nor when running it in parallel (using the -no-remote switch) with my usual SeaMonkey build and on a copy (made a few days ago) of its profile. The latter is a kind of stress test since it means running two instances in parallel with 68 and 78 tabs, using 714 and 1066 megabytes (while the "next biggest" program, Sunbird, uses "only" 264), and with some 38 extensions (of which 8 disabled) and 12 themes.

The "SeaMonkey Debug & QA UI" extension, which was hidden before when first accessing the Add-ons manager, does now appear (as well as all others AFAICT).

Feel free to REOPEN with details if you can reproduce the bug in a trunk build dated later than comment #15.

	Tony Mechelynck [:tonymec]
	Comment 17 • 14 years ago

oops... some eleven hours after, forgot to correct for time zone.