730551 - [SeaMonkey] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js | an unexpected uncaught JS exception reported through window.onerror - doc.getElementById("enabled") is null at ..."

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330135221.1330139643.6408.gz
Linux comm-aurora debug test mochitest-other on 2012/02/24 18:00:21  
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330111780.1330116495.23363.gz
WINNT 5.2 comm-aurora debug test mochitest-other on 2012/02/24 11:29:40
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330121294.1330126569.11375.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/24 14:08:14
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330125193.1330132229.22777.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/24 15:13:13
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js | an unexpected uncaught JS exception reported through window.onerror - doc.getElementById("enabled") is null at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js:21
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js | Found a tab after previous test timed out: http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/enabled.html
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js | Found a tab after previous test timed out: http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/bug638292.html
}

I reproduce on my local Windows 2000.

NB: I assume SeaMonkey 2.8 (= mozilla11) is affected too, though our current log stops (long) before executing this test :-/

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads [Checked in: Comments 3 and 16]

On my local Windows 2000, with Opt builds:
FF 13 does 0 intermediate load, SM 2.10 does 2(-3) intermediate loads.

Asking Neil, just as a reminder whether SeaMonkey could get rid of that behavior :-| (I expect the answer to (still) be 'no', but...)

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 600648 [details] [diff] [review]
(Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads
[Checked in: Comments 3 and 16]

passing the review over to Blair since my review queue is rather full atm.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 600648 [details] [diff] [review]
(Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads
[Checked in: Comments 3 and 16]

https://hg.mozilla.org/mozilla-central/rev/d1b2fd680235


[Approval Request Comment]
Regression caused by (bug #): Bug 638292.
User impact if declined: None, but perma-orange (timeout) on SeaMonkey.
Testing completed (on m-c, etc.): Comment 1 and this comment.
Risk to taking this patch (and alternatives if risky): None, test only.
String changes made by this patch: None.

Comment 4

[[e-mail]]

Comment on attachment 600648 [details] [diff] [review]
(Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads
[Checked in: Comments 3 and 16]

Sorry for overlooking the feedback request.

These aren't about:blank loads, but the test was wrong in thinking that its capturing load event handler would only capture the load event that it was interested in... the correct test is something like
if (aEvent.target != gBrowser.browsers[2].contentDocument)
  return;

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> https://hg.mozilla.org/mozilla-central/rev/d1b2fd680235

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330337923.1330342430.26680.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/27 02:18:43

V.Fixed

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) browser_bug638292.js: Make SeaMonkey support more specific

(In reply to [e-mail] from comment #4)

> Sorry for overlooking the feedback request.

(No problem.)

> These aren't about:blank loads, but the test was wrong in thinking that its

Wrong guess on my part :-/

> capturing load event handler would only capture the load event that it was

Ftr, the additional SM events have |aEvent.target.localName = "tabbrowser"| and the expected SM load event has 'null'.

> interested in... the correct test is something like
> if (aEvent.target != gBrowser.browsers[2].contentDocument)

Ftr, gBrowser.browsers[2] is null the very first time.

Comment 7

[[e-mail]]

(In reply to Serge Gautherie from comment #6)
> > capturing load event handler would only capture the load event that it was
> Ftr, the additional SM events have |aEvent.target.localName = "tabbrowser"|
More usefully, aEvent.originalTarget.localName == "image"

> and the expected SM load event has 'null'.
Well, the expected event is a document.

> Ftr, gBrowser.browsers[2] is null the very first time.
Ah, so maybe not such a good idea then.

Comment 8

[[e-mail]]

Comment on attachment 600865 [details] [diff] [review]
(Bv1) browser_bug638292.js: Make SeaMonkey support more specific

Personally I think this is overkill, but if I think of a better test than !aEvent.target.location then I'll let you know.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 600865 [details] [diff] [review]
(Bv1) browser_bug638292.js: Make SeaMonkey support more specific

(In reply to [e-mail] from comment #7)

> More usefully, aEvent.originalTarget.localName == "image"

What is this image(s)? Would there be a way to disable its load(s)?

> > Ftr, gBrowser.browsers[2] is null the very first time.
> Ah, so maybe not such a good idea then.

Yeah, that's why I ended up using "!aEvent.target.location" :-|


(In reply to [e-mail] from comment #8)
> Personally I think this is overkill

As my initial documentation (wrt "about:blank") was wrong, let's improve it.
In addition, I (now) think it's better to check for SeaMonkey too, to keep default case as strict as it was. (= Let it fail in case of a future regression.)
Wrt gBrowser.browsers[2] conditions, as well be more explicit...

Comment 10

[[e-mail]]

(In reply to Serge Gautherie from comment #9)
> (In reply to [e-mail] from comment #7)
> > More usefully, aEvent.originalTarget.localName == "image"
> What is this image(s)? Would there be a way to disable its load(s)?
It's the image element on the tab that displays the favicon or default icon.

> (In reply to [e-mail] from comment #8)
> > Personally I think this is overkill
> As my initial documentation (wrt "about:blank") was wrong, let's improve it.
> In addition, I (now) think it's better to check for SeaMonkey too, to keep
> default case as strict as it was. (= Let it fail in case of a future
> regression.)
> Wrt gBrowser.browsers[2] conditions, as well be more explicit...
Well I suggest that's really up to the module owner...

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 600865 [details] [diff] [review]
(Bv1) browser_bug638292.js: Make SeaMonkey support more specific

(In reply to [e-mail] from comment #10)
> (In reply to Serge Gautherie from comment #9)
> > What is this image(s)? Would there be a way to disable its load(s)?
> It's the image element on the tab that displays the favicon or default icon.

(Ftr/fwiw, I tried to disable browser.chrome.favicons and browser.chrome.site_icons, but that didn't help.)

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 600648 [details] [diff] [review]
(Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads
[Checked in: Comments 3 and 16]

[Triage Comment]
Test only change - approved for Aurora 12 and Beta 11.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 600865 [details] [diff] [review]
(Bv1) browser_bug638292.js: Make SeaMonkey support more specific

Review of attachment 600865 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js
@@ +13,5 @@
>  
>  function check_load(aCallback) {
>    gBrowser.addEventListener("load", function(aEvent) {
> +    // SeaMonkey tabbrowser needs to deal with additional loads.
> +    if (navigator.userAgent.match(/ SeaMonkey\//) &&

I really like to avoid application-specific code, even in tests. And having it test it's the right document seems proper anyway.

Why is gBrowser.browsers[2] null the first time?

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) browser_bug638292.js: Check load target, Make SeaMonkey support more specific [Checked in: Comment 21]

Bv1, with comment 13 suggestion(s).


(In reply to Blair McBride (:Unfocused) from comment #13)

> I really like to avoid application-specific code, even in tests. And having

Well, SeaMonkey has some specific behaviors, so it's needed sometimes.

> it test it's the right document seems proper anyway.

Done.

> Why is gBrowser.browsers[2] null the first time?

I have no idea wrt the exact order of things on SeaMonkey. Just accounting for it.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Serge Gautherie (:sgautherie) from comment #14)
> > Why is gBrowser.browsers[2] null the first time?
> 
> I have no idea wrt the exact order of things on SeaMonkey. Just accounting
> for it.

Any idea about this, Neil?

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 600648 [details] [diff] [review]
(Av1) browser_bug638292.js: Support SeaMonkey's "about:blank" loads
[Checked in: Comments 3 and 16]

http://hg.mozilla.org/releases/mozilla-aurora/rev/27165fe21717
http://hg.mozilla.org/releases/mozilla-beta/rev/1f861eb71d15

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16)

> http://hg.mozilla.org/releases/mozilla-beta/rev/1f861eb71d15

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1330489986.1330493973.10985.gz
OS X 10.6 comm-beta debug test mochitest-other on 2012/02/28 20:33:06

seamonkey2.8: verified.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/27165fe21717

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330509264.1330514338.13867.gz
Linux comm-aurora debug test mochitest-other on 2012/02/29 01:54:24

seamonkey2.9: verified.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #15)
> (In reply to Serge Gautherie (:sgautherie) from comment #14)
> > > Why is gBrowser.browsers[2] null the first time?
> > 
> > I have no idea wrt the exact order of things on SeaMonkey. Just accounting
> > for it.
> 
> Any idea about this, Neil?

Neil said this may not be unexpected.

Comment 20

[Mozilla RelEng Bot]

Try run for fb519344328a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fb519344328a
Results (out of 27 total builds):
    success: 21
    warnings: 4
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fb519344328a

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 601221 [details] [diff] [review]
(Bv2) browser_bug638292.js: Check load target, Make SeaMonkey support more specific
[Checked in: Comment 21]

https://hg.mozilla.org/mozilla-central/rev/14099190f012


(In reply to Mozilla RelEng Bot from comment #20)
>     https://tbpl.mozilla.org/?tree=Try&rev=fb519344328a

Succeeded.

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #21)
> https://hg.mozilla.org/mozilla-central/rev/14099190f012

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331104973.1331111681.4909.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2012/03/06 23:22:53

V.Fixed wrt this additional patch too.

Comment 23

[[e-mail]]

(In reply to Blair McBride from comment #13)
> Why is gBrowser.browsers[2] null the first time?
You're actually seeing the load of the icon for the bug638292.html tab (remember that it just loaded, so we're switching from the loading icon to the default icon). I believe link clicking is asynchronous so that the new tab isn't created until after the event loop processes the icon load event.