Closed Bug 724331 Opened 13 years ago Closed 13 years ago

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

Categories

(SeaMonkey :: Page Info, defect)

Product:
SeaMonkey
Component:
Page Info
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(seamonkey2.8 wontfix, seamonkey2.9 verified)

Status:
VERIFIED FIXED
Milestone:
seamonkey2.10
Tracking Flags:
Tracking Status
seamonkey2.8 --- wontfix
seamonkey2.9 --- verified

People

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

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Whiteboard: [perma-orange])

Attachments

(1 file)

Patch v1.0 Proposed fix [Checked in: See comment 10 & 19]
2.65 KB, patch
neil
: review+
sgautherie
: feedback+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
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: philip.chee → nobody
Component: Testing Infrastructure → Page Info
QA Contact: testing-infrastructure → page-info
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)
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?
>>- 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
URL: http://mxr.mozilla.org/comm-central/f...
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.
status-seamonkey2.8: --- → wontfix
status-seamonkey2.9: --- → wontfix
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"
> 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: 13 years ago
Resolution: --- → FIXED
> status-seamonkey2.9: --- → wontfix > status-seamonkey2.8: --- → wontfix Ah, is our policy not to backport fixes to tests?
status-seamonkey2.10: --- → verified
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.
status-seamonkey2.10: verified → ---
(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?
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.
status-seamonkey2.9: fixed → 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.

Attachment

General

Creator:
Created:
Updated:
Size: