Tabview assert spew in test logs "tabview assert: should already be linked"

RESOLVED WONTFIX

Status

Firefox Graveyard
Panorama
P2
normal
RESOLVED WONTFIX
8 years ago
2 years ago

People

(Reporter: mossop, Assigned: iangilman)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Looking at even a passing test run like this: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284482369.1284483351.30642.gz&fulltext=1 shows lots and lots of tabview assertions like:

tabview assert: should already be linked([object XULElement],[object Event])@chrome://browser/content/tabview.js:4766
((function (tab) {if (tab.ownerDocument.defaultView != gWindow || tab.pinned) {return;}self.update(tab);}),1,[object Array])@resource:///modules/tabview/AllTabs.jsm:127
([object Event])@resource:///modules/tabview/AllTabs.jsm:125
_tabAttrModified([object XULElement])@chrome://browser/content/tabbrowser.xml:895
updateCurrentBrowser()@chrome://browser/content/tabbrowser.xml:862
onselect([object Event])@chrome://browser/content/browser.xul:1
set_selectedIndex(0)@chrome://global/content/bindings/tabbox.xml:654
set_selectedPanel([object XULElement])@chrome://global/content/bindings/tabbox.xml:673
set_selectedIndex(0)@chrome://global/content/bindings/tabbox.xml:394
set_selectedItem([object XULElement])@chrome://global/content/bindings/tabbox.xml:426
set_selectedTab([object XULElement])@chrome://global/content/bindings/tabbox.xml:106
set_selectedTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1902
_blurTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1648
_endRemoveTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1517
removeTab([object XULElement],(void 0))@chrome://browser/content/tabbrowser.xml:1403
removeCurrentTab()@chrome://browser/content/tabbrowser.xml:1359
testExpertPref([object Event])@chrome://mochikit/content/browser/browser/components/certerror/test/browser_bug431826.js:37
(Assignee)

Updated

8 years ago
Priority: -- → P2
(Assignee)

Comment 1

8 years ago
Created attachment 475290 [details] [diff] [review]
patch v1

This assert is being triggered because tabbrowser.xml can send TabAttrModified on tabs even after it sends out close events for them. Basically it's a false alarm, so I propose replacing the assert with an if.
Attachment #475290 - Flags: feedback?(mitcho)
(In reply to comment #1)
> This assert is being triggered because tabbrowser.xml can send TabAttrModified
> on tabs even after it sends out close events for them.

You should probably file a bug on that.
Comment on attachment 475290 [details] [diff] [review]
patch v1

feedback+. But please also follow through on creating a bug as per Dāo.
Attachment #475290 - Flags: feedback?(mitcho) → feedback+
(Assignee)

Updated

8 years ago
Attachment #475290 - Flags: review?(dietrich)
(Assignee)

Comment 4

8 years ago
Follow-up bug 596687 filed. 

Dietrich, this seems like such a small change I shouldn't have to send it to the try server (assuming it passes tests locally); would you concur, or should we always send to try?
Can we just fix bug 596687 instead? I'd rather not wallpaper this, if we can fix the root cause.
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> Can we just fix bug 596687 instead? I'd rather not wallpaper this, if we can
> fix the root cause.

The assert isn't causing any harm, except for junking up the logs (and making people think there's a problem when there isn't), so I suppose we can let it ride for a bit. If bug 596687 doesn't get fixed for a while though, we should probably go ahead and apply the wallpaper.
Assignee: nobody → ian
Comment on attachment 475290 [details] [diff] [review]
patch v1

Canceling review. update() is called when it absolutely should not be called (IIUC). Let's not throw bad code on top of bad code. You should close this, and just fix the root cause yourself and get Dao to review it :)
Attachment #475290 - Flags: review?(dietrich)

Updated

8 years ago
Depends on: 596687
(Assignee)

Comment 8

8 years ago
Resolving as won't fix; bug 596687 has been fixed, so we shouldn't be seeing the assert any more.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX

Updated

4 years ago
See Also: → bug 987701
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.