Closed Bug 585511 Opened 14 years ago Closed 14 years ago

Icons sometimes set on wrong tab

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a3

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(2 files)

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.
blocking-seamonkey2.1: --- → ?
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()!
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #463982 - Flags: review?(neil)
Attachment #463982 - Flags: review?(bugspam.Callek)
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!
Depends on: 554908
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.
Attachment #463982 - Flags: review?(neil)
Attachment #463982 - Flags: review?(bugspam.Callek)
Attachment #463982 - Flags: review+
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...
Attachment #463982 - Flags: review-
(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.
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! ;-)
Attachment #464070 - Flags: review?(bugspam.Callek)
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.]
Attachment #463982 - Flags: review-
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.
Attachment #464070 - Flags: review?(bugspam.Callek) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
blocking-seamonkey2.1: ? → ---
Depends on: 586055
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.