Last Comment Bug 585511 - Icons sometimes set on wrong tab
: Icons sometimes set on wrong tab
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Robert Kaiser
:
Mentors:
Depends on: 554908 586055
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-08 17:48 PDT by Robert Kaiser
Modified: 2010-08-11 08:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: move invalidation, do some investigation-related cleanup (5.80 KB, patch)
2010-08-08 17:58 PDT, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
test for this bug (2.16 KB, patch)
2010-08-09 09:03 PDT, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-08 17:48:43 PDT
Ever since my work on bug 554908 I watch tab icons more closely, and every now and then saw icons be magically shifted one tab to the right.

Today, finally, I could reproduce it as much as to find out that when this happens, the indices of .tabs and .browsers in tabbrowser are out of sync, and so http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1283 sets its icon on the wrong tab. I also found out that it always happens when a tab left to the one being reloaded had been closed before and none added.
This lead me to realize that the problem must be with _browsers invalidation in the removeTab method, and adding a Tab, which also invalidates that cache, works correctly. I had done some small-ish not-completely-related cleanup in tabbrowser until I reached this point.
My final conclusion was/is that between the current invalidation in http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1698 and the actual removal of the tab from the tabContainer, we call something (when dispatching the TabClose event by chance?) that reads .browsers and therefore reconstructs the cache in the old to-be-obsoleted state. Firefox does the invalidation right before it removes the tab from the container, and when I moved it there, I couldn't reproduce the failure any more.

Now that I know how to reproduce, I always can, by the way (in a build before my change):
1) Open a few tabs with different icons.
2) Close one in the middle of the tab bar.
3) Go to the one just right of where the closed one was and hit reload.
4) See the icon this tab should have appear to the one right of it.
Comment 1 Robert Kaiser 2010-08-08 17:58:23 PDT
Created attachment 463982 [details] [diff] [review]
v1: move invalidation, do some investigation-related cleanup

Here's a patch that fixes this and does some additional cleanup I came across while investigating this.

The real fix is the move of |this._browsers = null| to the later place, I also adjusted the comment to the same that's used by Firefox, as it explains much better why we are doing that right there.

The first three hunks clean up the onLinkAdded stuff a bit wrt to creating URI objects, making use of Services and removing that dependency on makeURI I introduced, as it's really unneeded.

The last hunk makes the actual retrieval of .browsers and buildup of that _browsers cache much nicer, this is taken 1:1 from Firefox and much simpler than what we had before with the use of .map()!
Comment 2 Robert Kaiser 2010-08-08 17:59:25 PDT
Oh, forgot to say, I'll take the review from whoever of you two is getting to it first, please cancel the other one when you comment on the patch!
Comment 3 Justin Wood (:Callek) 2010-08-08 19:26:54 PDT
Comment on attachment 463982 [details] [diff] [review]
v1: move invalidation, do some investigation-related cleanup

Nit: if its not too hard to test, I'd love a browser-chrome test for this bug, so we can be sure not to regress.
Comment 4 neil@parkwaycc.co.uk 2010-08-09 01:42:18 PDT
Comment on attachment 463982 [details] [diff] [review]
v1: move invalidation, do some investigation-related cleanup

>-              browsers.item = function(i) {return this[i];}
This line should not have been deleted...
Comment 5 Robert Kaiser 2010-08-09 06:38:36 PDT
(In reply to comment #4)
> >-              browsers.item = function(i) {return this[i];}
> This line should not have been deleted...

And why, please? Firefox doesn't need it, and I saw no errors in my testing without it, i.e. with this patch as it is.
Comment 6 Robert Kaiser 2010-08-09 09:03:48 PDT
Created attachment 464070 [details] [diff] [review]
test for this bug

Thanks, Callek, for the reminder that such things should have a test.
And here is one that fails without the patch and succeeds with it. And it's soooo easy! ;-)
Comment 7 neil@parkwaycc.co.uk 2010-08-09 09:18:57 PDT
Comment on attachment 463982 [details] [diff] [review]
v1: move invalidation, do some investigation-related cleanup

Withdrawing my r- since Ratty suggests that that line isn't necessary any more.

I don't really want to use Services.io but it's better than using makeURI.

[XBL fields are best because they are already lazy.]
Comment 8 Justin Wood (:Callek) 2010-08-09 12:08:35 PDT
Comment on attachment 464070 [details] [diff] [review]
test for this bug

This is good at testing for underlying bug, I was also hoping something could test the user-facing issue we experienced. (The site-icon on wrong tab), but if that's not easy, I'm not expecting it.
Comment 9 Robert Kaiser 2010-08-09 18:52:37 PDT
After some talk on IRC, Callek approved landing on the tree, given the fact that the bustage is probably just shared builds and static ones might succeed, i.e. we'll have regular nightlies, and we want this to be fixed there.
Pushed main patch as http://hg.mozilla.org/comm-central/rev/65c8c3ebfb11 and test as http://hg.mozilla.org/comm-central/rev/6d7d75ff7a84

Note You need to log in before you can comment on or make changes to this bug.