"this.childNodes[i].tab is undefined" errors every time the List All Tabs menu is opened

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: fryn, Assigned: fryn)

Tracking

Trunk
Firefox 6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The patch for bug 625320 broke the function _updateTabsVisibilityStatus in tabbrowser.xml, by adding non-anonymous child nodes to the List All Tabs menu.

Now we get the error "this.childNodes[i].tab is undefined" every time the List All Tabs menu is opened.

I'll fix this, since I need it working for another patch I want to land.

(I noticed that the patch in bug 626854 fixes this too, but we should land this fix regardless of that bug's atatus.)
(Assignee)

Updated

6 years ago
Blocks: 626903
(Assignee)

Comment 1

6 years ago
Created attachment 529039 [details] [diff] [review]
patch

Setting dolske as reviewer, since he reviewed Tim's patch for the main bug.
Attachment #529039 - Flags: review?(dolske)
(Assignee)

Comment 2

6 years ago
Created attachment 529051 [details] [diff] [review]
patch v2

This version adds comments and takes into account Sync UI doing bizarre stuff.
See bug 626903 comment 4.
Attachment #529039 - Attachment is obsolete: true
Attachment #529039 - Flags: review?(dolske)
Attachment #529051 - Flags: review?(dolske)

Comment 3

6 years ago
Comment on attachment 529039 [details] [diff] [review]
patch

Review of attachment 529039 [details] [diff] [review]:

Note that browser-syncui.js adds a fake tab object without a boxObject and a mCorrespondingMenuitem:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-syncui.js#161

::: browser/base/content/tabbrowser.xml
@@ +3947,5 @@
           for (var i = 0; i < this.childNodes.length; i++) {
+            let curTab = this.childNodes[i].tab;
+            if (!curTab)
+              continue;
+            let curTabBO = curTab.boxObject;

So this will fail for syncui's fake tab object because boxObject is not defined.

@@ +4029,2 @@
             menuItem.removeEventListener("command", this, false);
             menuItem.tab.mCorrespondingMenuitem = null;

And this will fail for syncui's fake tab object because mCorrespondingMenuitem is not defined.

So you either need to add checks in both places, or remove the fake tab object from syncui but still take care of removeChild'ing sync-tabs-menuitem here. My patch in bug 626854 does the latter by still relying on the "keepme" attribute.
Attachment #529039 - Attachment is obsolete: false

Comment 4

6 years ago
Oh, I noticed your second patch too late.

Please check if popuphidden's
  menuItem.tab.mCorrespondingMenuitem = null;
doesn't fail for syncui's fake tab object, which has no mCorrespondingMenuitem.
(Assignee)

Comment 5

6 years ago
(In reply to comment #4)
> Please check if popuphidden's
>   menuItem.tab.mCorrespondingMenuitem = null;
> doesn't fail for syncui's fake tab object, which has no mCorrespondingMenuitem.

It won't fail. JavaScript objects support expandos.

Comment 6

6 years ago
Yeah, I somehow remembered hitting an error there when writing the patch for bug 626854. But the spec says that removeEventListener doesn't throw either.
(Assignee)

Updated

6 years ago
Attachment #529039 - Attachment is obsolete: true
Attachment #529051 - Flags: review?(dolske) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

6 years ago
Created attachment 532416 [details] [diff] [review]
hg export of patch v2 for checkin [r=dolske]
http://hg.mozilla.org/mozilla-central/rev/48c0f02f4614
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6

Comment 9

6 years ago
 Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on WinXP, Mac OS X 10.6 and Ubuntu 10.10 - no errors present in the Error Console when opening List all tabs.

Setting status to Verified Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.