Closed Bug 653655 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 6

People

(Reporter: fryn, Assigned: fryn)

References

Details

Attachments

(2 files, 1 obsolete file)

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.)
Blocks: 626903
Attached patch patch (obsolete) — Splinter Review
Setting dolske as reviewer, since he reviewed Tim's patch for the main bug.
Attachment #529039 - Flags: review?(dolske)
Attached patch patch v2Splinter Review
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 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
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.
(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.
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.
Attachment #529039 - Attachment is obsolete: true
Attachment #529051 - Flags: review?(dolske) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/48c0f02f4614
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
 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.