521216 - Tab is Undefined error in Error Console - Win7 Previews

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

This error is appearing in the Console2 Error Console:

Error: tab is undefined
Source file: file:///J:/Program%20Files%20(x86)/Minefield/modules/WindowsPreviewPerTab.jsm
Line: 514

Not sure yet of what's triggering the error.  Seems it might be related to sites doing a periodic refresh of page, i.e.: msnbc perhaps.  

Using an hourly build:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091007231829

cset: http://hg.mozilla.org/mozilla-central/rev/d8c914dbd3f8

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

As I clicked 'commit' while filing this bug I got the error to appear.

Comment 2

[Dão Gottwald [:dao]]

Rob: 'for each' shouldn't be used like this. There's no good reason to use it on arrays, and it's plain wrong on node lists. See also the warning here: <https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for_each...in#Examples>

Comment 3

[Dão Gottwald [:dao]]

fixing...

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Comment 5

[Rob Arnold [:robarnold]]

Comment on attachment 405251 [details] [diff] [review]
patch

Looks good. Wish I'd known that sooner!

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f4c973091e28

Comment 7

[Ed Lee :Mardak]

Comment on attachment 405251 [details] [diff] [review]
patch

>@@ -367,18 +367,18 @@ XPCOMUtils.defineLazyGetter(PreviewContr
>-  for each (let evtName in this.events)
>-    this.tabbrowser.tabContainer.addEventListener(evtName, this, false);
>+  for (let i = 0; i < this.events.length; i++)
>+    this.tabbrowser.tabContainer.addEventListener(this.events[i], this, false);
Why not use .forEach? Is that not an actual array []?

Array.forEach(this.events, function(event) {
  this.tabbrowser.tabContainer.addEventListener(event, this, false);
}, this);

Comment 8

[Ed Lee :Mardak]

Oh, it is a plain array, so this.events.forEach should work

Comment 9

[Dão Gottwald [:dao]]

Yes, that it would have worked as well. Since it's a one-liner (allowing me to drop the brackets) and this.events[i] appears only once, I don't think forEach would have improved the code significantly. I also wonder if forEach is actually still faster.

Comment 10

[Jim Jeffery not reading bug-mail 1/2/11]

Ok, I just downloaded the latest hourly that should contain this patch, and I still see the error..

cset: http://hg.mozilla.org/mozilla-central/rev/542fa9413bd0

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre Firefox/3.7 (.NET CLR 3.5.30729) ID:20091008114812

Error: this.previews[index] is undefined
Source file: file:///J:/Program%20Files%20(x86)/Minefield/modules/WindowsPreviewPerTab.jsm
Line: 514

Comment 11

[Jim Jeffery not reading bug-mail 1/2/11]

I can still trigger the error by simply opening a bug from history.

Or middle-clicking a link from the location bar.

Comment 12

[Dão Gottwald [:dao]]

Landed this to get more useful information:
http://hg.mozilla.org/mozilla-central/rev/7e78bc8c98ea

Please report back with new console output.

Comment 13

[[not reading bugmail]]

(In reply to comment #10)
> Ok, I just downloaded the latest hourly that should contain this patch, and I
> still see the error..
> 
> cset: http://hg.mozilla.org/mozilla-central/rev/542fa9413bd0
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008
> Minefield/3.7a1pre Firefox/3.7 (.NET CLR 3.5.30729) ID:20091008114812
> 
> Error: this.previews[index] is undefined
> Source file:
> file:///J:/Program%20Files%20(x86)/Minefield/modules/WindowsPreviewPerTab.jsm
> Line: 514

Did you clean the error counsel first before installing the new build?

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

@ cuz84d
Yes, I always clear the console2 before starting to test for bugs like this.

I just got the build with debug patch and here is the output:
1.  Middle-clicked a link from the location-bar to open in a tab

Error: 13,14,14,13
Source file: file:///J:/Program%20Files%20(x86)/Minefield/modules/WindowsPreviewPerTab.jsm
Line: 517

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

The error seems to change, here is another from mid-click on link from location-bar drop list:

Error: 11,12,12,11
Source file: file:///J:/Program%20Files%20(x86)/Minefield/modules/WindowsPreviewPerTab.jsm
Line: 517

Comment 16

[Dão Gottwald [:dao]]

This might be a tabbrowser bug. I've pushed something to the tryserver for testing.

Comment 17

[Dão Gottwald [:dao]]

Build is here: https://build.mozilla.org/tryserver-builds/[e-mail]-try-115ca7ebbd70/try-115ca7ebbd70-win32.zip

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #17)
> Build is here:
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-115ca7ebbd70/try-115ca7ebbd70-win32.zip

This build is good, no errors...

Comment 19

[Dão Gottwald [:dao]]

Attachment: tabbrowser patch

So the problem is that the tabs progress listener gets notifications about the new browser before the TabOpen event is dispatched.

Comment 20

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #19)
> Created an attachment (id=405455) [details]
> tabbrowser patch
> 
> So the problem is that the tabs progress listener gets notifications about the
> new browser before the TabOpen event is dispatched.

Not to butt in, but you didn't mark this for review ?

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 405455 [details] [diff] [review]
tabbrowser patch

passed all unit tests on the tryserver

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b2057b7554e6

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405455 [details] [diff] [review]
tabbrowser patch

a192=beltzner

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/53d5343f7326
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1e2aea110399

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

why is this still checkin-needed? what's left to do?

Comment 26

[Shawn Wilsher :sdwilsh]

I had to take these on the release branch so bug 522416 would apply cleanly and I didn't have to figure out a special patch just for release branch for beta 1.  If this was wrong of me to do, I'm sorry and we can back it out.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/27d9d4fb4064
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a5340b714f5f

Comment 27

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.2 branch using  Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b6pre) Gecko/20100105 Namoroka/3.6b6pre. I verified by opening links from the location bar with middle clicks and opening history entries.