Last Comment Bug 653655 - "this.childNodes[i].tab is undefined" errors every time the List All Tabs menu is opened
: "this.childNodes[i].tab is undefined" errors every time the List All Tabs men...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Frank Yan (:fryn)
:
Mentors:
Depends on:
Blocks: 625320 626903 639730
  Show dependency treegraph
 
Reported: 2011-04-28 22:09 PDT by Frank Yan (:fryn)
Modified: 2011-07-27 07:39 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.18 KB, patch)
2011-04-28 22:22 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v2 (3.32 KB, patch)
2011-04-29 00:33 PDT, Frank Yan (:fryn)
dolske: review+
Details | Diff | Splinter Review
hg export of patch v2 for checkin [r=dolske] (3.32 KB, patch)
2011-05-13 23:09 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review

Description Frank Yan (:fryn) 2011-04-28 22:09:31 PDT
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.)
Comment 1 Frank Yan (:fryn) 2011-04-28 22:22:32 PDT
Created attachment 529039 [details] [diff] [review]
patch

Setting dolske as reviewer, since he reviewed Tim's patch for the main bug.
Comment 2 Frank Yan (:fryn) 2011-04-29 00:33:36 PDT
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.
Comment 3 Steffen Wilberg 2011-04-29 01:07:13 PDT
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.
Comment 4 Steffen Wilberg 2011-04-29 01:10:58 PDT
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.
Comment 5 Frank Yan (:fryn) 2011-04-29 01:13:10 PDT
(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 Steffen Wilberg 2011-04-29 01:38:13 PDT
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.
Comment 7 Frank Yan (:fryn) 2011-05-13 23:09:12 PDT
Created attachment 532416 [details] [diff] [review]
hg export of patch v2 for checkin [r=dolske]
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-05-15 07:51:13 PDT
http://hg.mozilla.org/mozilla-central/rev/48c0f02f4614
Comment 9 George Carstoiu 2011-07-27 07:39:34 PDT
 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.

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