TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | an unexpected uncaught JS exception reported through window.onerror - executeSoon is not defined at ..../suite/browser/test/browser_bug427559.js:56

VERIFIED FIXED in seamonkey2.10

Status

defect
--
major
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
seamonkey2.10
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

(seamonkey2.8 wontfix, seamonkey2.9 verified)

Details

(Whiteboard: [perma-orange], )

Attachments

(1 attachment)

Assignee

Description

8 years ago
Running this test locally and I get a consistent failure:

> *** Start BrowserChrome Test Results ***
> TEST-INFO | checking window state
> TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js
> Document data:text/html,<body><button%20onblur="this.parentNode.removeChild(this);"><script>document.body.firstChild.focus();</script></body> loaded successfully
> TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | content window is focused
> INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | finished in 519ms
> TEST-INFO | checking window state
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js |
>  an unexpected uncaught JS exception reported through window.onerror - executeSoon is not defined at
>  chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js:56
> Stack trace:
>     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 968
>     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
> 
> TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js |
>  Console message: [JavaScript Error: "executeSoon is not defined" {file: "chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js" line: 56}]
> 
> INFO TEST-START | Shutdown
> Browser Chrome Test Summary
>         Passed: 1
>         Failed: 1
>         Todo: 0
> 
> *** End BrowserChrome Test Results ***
Assignee

Updated

8 years ago
Assignee: philip.chee → nobody
Component: Testing Infrastructure → Page Info
QA Contact: testing-infrastructure → page-info
Assignee

Comment 1

8 years ago
After Porting the following changes from Firefox, the test passes:

http://hg.mozilla.org/mozilla-central/rev/5d22feabe471
tests cleanup

http://hg.mozilla.org/mozilla-central/rev/50a858927fad
Cleanup leftover listeners from browser/base/content browser-chrome tests


*** Start BrowserChrome Test Results ***
TEST-INFO | checking window state
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js
Document data:text/html,<body><button%20onblur="this.parentNode.removeChild(this);"><script>document.body.firstChild.focus();</script></body> loaded successfully
TEST-PASS | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | content window is focused
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | finished in 783ms
TEST-INFO | checking window state

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 1
        Failed: 0
        Todo: 0

*** End BrowserChrome Test Results ***
Assignee: nobody → philip.chee
Attachment #594507 - Flags: review?(neil)
Assignee

Updated

8 years ago
Attachment #594507 - Flags: feedback?(sgautherie.bz)
Comment on attachment 594507 [details] [diff] [review]
Patch v1.0 Proposed fix
[Checked in: See comment 10 & 19]

>+  gBrowser.selectedBrowser.addEventListener("load", function () {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
Named event listener please.

>-      is(document.commandDispatcher.focusedWindow, window.content,
>+      is(document.commandDispatcher.focusedWindow, testPageWin,
Why this change?
Assignee

Comment 3

8 years ago
>>-      is(document.commandDispatcher.focusedWindow, window.content,
>>+      is(document.commandDispatcher.focusedWindow, testPageWin,
>> Why this change?
Um, dunno. I just copied Dao's Firefox changes wholesale. Perhaps he can remember why he did things this way three years ago. cc Dão.
(In reply to Philip Chee from comment #0)
> Running this test locally and I get a consistent failure:

Confirmed on SM tinderboxes:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328412049.1328416873.12484.gz
OS X 10.5 comm-central-trunk debug test mochitest-other on 2012/02/04 19:20:49
Blocks: SmTestFail
Severity: normal → major
Whiteboard: [perma-orange]
Comment on attachment 594507 [details] [diff] [review]
Patch v1.0 Proposed fix
[Checked in: See comment 10 & 19]

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

::: suite/browser/test/browser/browser_bug427559.js
@@ +50,4 @@
>  
> +  gBrowser.selectedBrowser.addEventListener("load", function () {
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
> +    executeSoon(function () {

Yes, I prefer to keep executeSoon() if that still works :-)

@@ +50,5 @@
>  
> +  gBrowser.selectedBrowser.addEventListener("load", function () {
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
> +    executeSoon(function () {
> +      var testPageWin = content;

I guess adding
"// Save a reference to current 'content', to compare to it later."
might answer Neil's question,
as in doing a stricter comparison than to (later) "window.content".
Or is my guess just wrong?

@@ +61,2 @@
>  
>        // Make sure focus is given to the window because the element is now gone

Nit: this could use an ending '.'. (Though FF doesn't have it...)

@@ +66,3 @@
>        gBrowser.removeCurrentTab();
>        finish();
> +    }, 0);

You missed not to re-add ", 0".
Attachment #594507 - Flags: feedback?(sgautherie.bz) → feedback+
Just ftr,

SM 2.9:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1328399935.1328404279.22761.gz
OS X 10.5 comm-aurora debug test mochitest-other on 2012/02/04 15:58:55
SM 2.8:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1328288802.1328292418.23491.gz
OS X 10.5 comm-beta debug test mochitest-other on 2012/02/03 09:06:42

are affected too.
Comment on attachment 594507 [details] [diff] [review]
Patch v1.0 Proposed fix
[Checked in: See comment 10 & 19]

>+  gBrowser.selectedBrowser.addEventListener("load", function () {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

>-      is(document.commandDispatcher.focusedWindow, window.content,
>+      is(document.commandDispatcher.focusedWindow, testPageWin,
r=me with the function given a name (instead of using deprecated arguments.callee) and retain window.content instead of using the temporary variable.
Attachment #594507 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #7)
> retain window.content instead of using the temporary variable.

Neil, what do you think about my comment 5, especially wrt 'testPageWin'?
(In reply to neil@parkwaycc.co.uk from comment #7)
> with the function given a name (instead of using deprecated
> arguments.callee)

I didn't know.
Should I file a bug about that?
http://mxr.mozilla.org/comm-central/search?string=arguments.callee&case=on&find=%2Fsuite%2F
"Found 101 matching lines in 38 files"
Assignee

Comment 10

8 years ago
> Yes, I prefer to keep executeSoon() if that still works :-)
WFM.
>>        // Make sure focus is given to the window because the element is now gone
> Nit: this could use an ending '.'. (Though FF doesn't have it...)
Fixed.
>> +    }, 0);
> You missed not to re-add ", 0".
Fixed.

>>+  gBrowser.selectedBrowser.addEventListener("load", function () {
>>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
> 
>>-      is(document.commandDispatcher.focusedWindow, window.content,
>>+      is(document.commandDispatcher.focusedWindow, testPageWin,
> r=me with the function given a name (instead of using deprecated
> arguments.callee) and retain window.content instead of using the temporary
> variable.
Fixed.

Nits fixed on checkin. Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/109a02643af0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee

Comment 11

8 years ago
> status-seamonkey2.9: --- → wontfix
> status-seamonkey2.8: --- → wontfix
Ah, is our policy not to backport fixes to tests?
Target Milestone: --- → seamonkey2.10
(In reply to Philip Chee from comment #11)
> Ah, is our policy not to backport fixes to tests?

Considering our bad situation wrt tests, my/the current "policy" is not to care about branches (for test-only patches), unless there is some added value: last (few) test to go green, avoid timeouts/aborts/etc, ...
(It's a compromise to stay focused on trunk, until we manage to come back to a much better situation. Rapid cycle + lack of (human) resources :-|)

""status-seamonkey2.10: --- → verified"
Not using "status-*" for trunk, not verified on tinderboxes yet.
(In reply to Serge Gautherie from comment #5)
> (From update of attachment 594507 [details] [diff] [review])
> > +    executeSoon(function () {
> Yes, I prefer to keep executeSoon() if that still works :-)
It's probably necessary, isn't it? I'm never sure with load events.

> > +      var testPageWin = content;
> I guess adding
> "// Save a reference to current 'content', to compare to it later."
> might answer Neil's question,
> as in doing a stricter comparison than to (later) "window.content".
> Or is my guess just wrong?
I tried the test with window.content and it seemed to work fine, so...

(In reply to Serge Gautherie from comment #9)
> (In reply to comment #7)
> > with the function given a name (instead of using deprecated arguments.callee)
> I didn't know.
https://developer.mozilla.org/en/JavaScript/Reference/Functions_and_function_scope/arguments/callee#Description

> Should I file a bug about that?
> http://mxr.mozilla.org/comm-central/search?string=arguments.callee&case=on&find=%2Fsuite%2F
> "Found 101 matching lines in 38 files"
No rush, we just need to change them as and when we patch those uses.
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328477395.1328482376.20135.gz&fulltext=1
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/05 13:29:55
{
INFO TEST-END | chrome://mochitests/content/browser/suite/browser/test/browser_bug427559.js | finished in 800ms
TEST-INFO | checking window state
TEST-START | chrome://mochitests/content/browser/suite/browser/test/browser_bug519216.js
}

V.Fixed
Status: RESOLVED → VERIFIED
(In reply to neil@parkwaycc.co.uk from comment #13)

> (In reply to Serge Gautherie from comment #5)
> > Yes, I prefer to keep executeSoon() if that still works :-)
> It's probably necessary, isn't it? I'm never sure with load events.

Yeah, I just meant "instead of using setTimeout()", not to investigate whether a delay was needed at all ;-)

> > I guess adding
> > "// Save a reference to current 'content', to compare to it later."
> > might answer Neil's question,
> > as in doing a stricter comparison than to (later) "window.content".
> > Or is my guess just wrong?
> I tried the test with window.content and it seemed to work fine, so...

I assume it works.
What I suggested was whether using testPageWin might let catch a future regression if focus was suddenly not on the same/initial window.content.
If you think testPageWin is unneeded, I think we should file a bug to remove it from the FF test, shouldn't we?

> (In reply to Serge Gautherie from comment #9)
> https://developer.mozilla.org/en/JavaScript/Reference/
> Functions_and_function_scope/arguments/callee#Description
> 
> No rush, we just need to change them as and when we patch those uses.

I filed bug 724441 as a GFB...
(In reply to Philip Chee from comment #1)

> http://hg.mozilla.org/mozilla-central/rev/5d22feabe471
> tests cleanup

I filed bug 724448 to port the (applicable) rest of that changeset.

> http://hg.mozilla.org/mozilla-central/rev/50a858927fad
> Cleanup leftover listeners from browser/base/content browser-chrome tests

I filed bug 724446 to port the (applicable) rest of that changeset.
Flags: in-testsuite+
Comment on attachment 594507 [details] [diff] [review]
Patch v1.0 Proposed fix
[Checked in: See comment 10 & 19]

[Approval Request Comment]
This is now (almost) the last m-b-c failure on beta/2.9.
Attachment #594507 - Flags: approval-mozilla-beta?
Comment on attachment 594507 [details] [diff] [review]
Patch v1.0 Proposed fix
[Checked in: See comment 10 & 19]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Attachment #594507 - Flags: approval-mozilla-beta? → approval-comm-beta?

Updated

7 years ago
Attachment #594507 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #594507 - Attachment description: Patch v1.0 Proposed fix. → Patch v1.0 Proposed fix [Checked in: See comment 10 & 19]
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1334576714.1334580472.4627.gz
OS X 10.5 comm-beta debug test mochitest-other on 2012/04/16 04:45:14

seamonkey2.9: verified.
Depends on: 745808
(In reply to Serge Gautherie (:sgautherie) from comment #5)
> Yes, I prefer to keep executeSoon() if that still works :-)
> 
> I guess adding
> "// Save a reference to current 'content', to compare to it later."
> might answer Neil's question,
> as in doing a stricter comparison than to (later) "window.content".
> Or is my guess just wrong?

"Bug 745808 Submitted".
You need to log in before you can comment on or make changes to this bug.