562899 - Switching quickly between different categories shows different types of addons in the same pane

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Fritz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100430 Minefield/3.7a5pre ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100430 Minefield/3.7a5pre ( .NET CLR 3.5.30729)

Themes and Extensions get loaded in same Add-on manager window with two scroll bars.  Screenshot attached.

Reproducible: Sometimes

Steps to Reproduce:
1. Open Firefox to add-ons manager tab.
2. Click extensions
3. Click themes quickly after

Comment 1

[Fritz]

Attachment: Note themes and extensions shown together (under themes tab) and two scroll bars

Comment 2

[Fritz]

I should note that I had about 50 tabs loading on startup which is probably what caused the lag that allows this to happen.  I can only produce on startup and could not produce once the all pages in browser have loaded.

Comment 4

[Fritz]

Half of this bug is a duplicate...  the other half is the fact that themes and add-ons showed up in the same window which isn't mentioned in the other bug.

Comment 5

[Fritz]

Attachment: This time Extensions duplicated, not themes and extensions on same page

Comment 6

[Fritz]

(I removed duplicate status and removed the scrollbar issue as a part of this bug.  Marked as minor)

This time instead of showing themes and extensions on the same page it shows the extensions doubled up (see screenshot).  Once again this happened while I was restarting browser loading ~50 tabs.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can you please attach your extensions.sqlite file to that bug? What happens if you are opening the addons manager after all tabs have been successfully loaded?

Comment 8

[Fritz]

Attachment: Extensions.sqlite

Comment 9

[Fritz]

If I load it after all tabs have loaded then it shows up normally.  It also shows up normally when I restore a session with about:addons up as long as I don't switch between extensions/themes/search as things are loading.  So, it is a minor (trivial?) issue that can easily be fixed by reloading the add-ons manager.

Comment 10

[Kevin Brosnan]

How is this not a duplicate?

Comment 11

[Kevin Brosnan]

Nevermind comment 10

Comment 12

[Fritz]

Attachment: Extensions and Search pages merged under Extensions

To produce this I went into the add-ons manager, clicked on extensions and pressed up and down and up and down, etc. very quickly until I noticed weirdness in the UI.  This was done not during load time but while the page was open with no pages loading.

Comment 13

[Fritz]

Actually, it's even simpler.  Just press up and down at the same time under any of the two add-ons categories and you will get a merging of those two categories.  My guess is that that is only happens more on startup because of program slowness makes the threshold time less.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was able to reproduce it with the steps in comment 13. Thanks Fritz! In my case I have even seen Adblock Plus listed 5 times in a row. Looks like it's an issue with the asynchronous execution.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #14)
> Looks like it's an issue with the asynchronous execution.

Indeed - the pane is being refreshed before the async calls to getAddonsByTypes() and getInstallsByTypes() complete. Need to somehow mark that these callbacks are no longer relevant. Should do that when the pane is hidden too.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Somehow I cannot reproduce this bug anymore. Blair, have we already checked-in some code which has been fixed that bug?

Comment 18

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Something has eased this a little, but it is still a problem. I have a patch ready

Comment 19

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: patch rev 1

This adds a request ID to the list view. If callbacks come back that aren't for the latest request then they are ignored. One a request succeeds we reset the ID. When closing the window we set the ID to -1 to stop any pending requests from trying to access the window.

I've added more code to mark views as loading so that the test harness can poll to detect when that happens and added some useful functions for opening the add-ons manager (in window or tab) and waiting for views to load. The test quickly loads two categories and checks the number in the resulting list is correct. This fails without the fix and passes with the fix.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

>+    this.node.setAttribute("loading", true);

I'd prefer an event over polling an attribute, but I guess it works. On the upside, its not just the detail view with the loading attribute now.


> var gListView = {
>   node: null,
>   _listBox: null,
>   _emptyNotice: null,
>   _sorters: null,
>   _types: [],
>   _installTypes: [],
>+  _request: 0,


This is per view object, so if we quickly between view objects (say, gListview and gSearchView), it wouldn't ignore old requests. It wouldn't show up though, because views (currently) don't share display elements - but it would still mean unnecessary processing. So I think the request ID should be incremented in each view's hide() as well.


>diff --git a/toolkit/mozapps/extensions/test/browser/browser_bug566915.js b/toolkit/mozapps/extensions/test/browser/browser_bug566915.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/browser/browser_bug566915.js
>@@ -0,0 +1,81 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// This tests simulated drag and drop of files into the add-ons manager.
>+// We test with the add-ons manager in its own tab if in Firefox otherwise
>+// in its own window.
>+// Tests are only simulations of the drag and drop events, we cannot really do
>+// this automatically.

The test file is named after one of the DUPE bugs, rather than this bug. That comment is also about a different bug (bug 565682 I'm guessing).

r=me, with those fixes.

Comment 21

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: patch rev 2

Made a bunch of changes here so this needs re-review I think. I'm using the event suggestion, that feels a bit nicer however I'm still adding the loading attribute to all views (through the view controller now) so that the test harness knows whether a view change is actually pending or not.

I've also switched away from the request counter and am just testing whether the current view is the same as the one the request was gathering data for. This is a scheme that works for all views and seems a bit simpler to me.

Note we have to check if gViewController exists to catch the case when the window has been closed and the JS context torn away before the callback is called.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 451611 [details] [diff] [review]
patch rev 2

I do like this much more in general, but...

If you switch between View A, View B, and View A again; and the 2nd time View A is shown it starts a new request before the first showing of View A completes, then we still get the same problem. So I think it still needs some kind of unique salt (gViewController.currentViewSalt ?).

Comment 23

[Dave Townsend [:mossop] (PTO until Aug 21st)]

(In reply to comment #22)
> (From update of attachment 451611 [details] [diff] [review])
> I do like this much more in general, but...
> 
> If you switch between View A, View B, and View A again; and the 2nd time View A
> is shown it starts a new request before the first showing of View A completes,
> then we still get the same problem. So I think it still needs some kind of
> unique salt (gViewController.currentViewSalt ?).

But in that scenario it doesn't matter that we display the results of the first request does it? The results should be the same for the same view right?

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #23)
> But in that scenario it doesn't matter that we display the results of the first
> request does it? The results should be the same for the same view right?

They'll be the same, but there would be 2 of every item.  Unless you want to clear the list in the callback function, in addition to already clearing the list before we do anything async (which would still be needed). Think a unique salt is the more general solution of the two, though.

Comment 25

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: patch rev 3

Went with currentViewRequest and added a test for that specific case. Also passing the request as a parameter to show seemed like a good idea.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 451799 [details] [diff] [review]
patch rev 3

As discussed on IRC, another edgecase:

select A (req == 1), select B (req == 2), B loads (req reset to 0), select C (req == 1), A loads

so r=me with the request ID not being reset in notifyViewChanged (with or without ignoring any overflow issues).

Comment 28

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Landed: http://hg.mozilla.org/mozilla-central/rev/09ee111097ae

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre