Closed Bug 1270704 Opened 4 years ago Closed 3 years ago

Intermittent e10s browser_browserLoaded_content_loaded.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW

Categories

(Testing :: Mochitest, defect, P5)

defect

Tracking

(e10s+, firefox49 wontfix, firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
e10s + ---
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: philor, Assigned: marcosc)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Blocks: e10s-tests
tracking-e10s: --- → +
Intermittent e10s test failure
Priority: -- → P5
Currently the fourthmost frequent dead CPOW orange in automation. Can you please take a look, Marcos?

TEST-UNEXPECTED-FAIL | testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js | Uncaught exception - at chrome://mochitests/content/browser/testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js:7 - Error: operation not possible on dead CPOW
Stack trace:
    isDOMLoaded@chrome://mochitests/content/browser/testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js:7:3
    @chrome://mochitests/content/browser/testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js:37:13
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
    Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
    this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
    this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
    Tester_execTest@chrome://mochikit/content/browser-test.js:784:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:704:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59

https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js#7
> return aBrowser.contentWindowAsCPOW.document.readyState === 'complete';
Blocks: 1154464
Flags: needinfo?(mcaceres)
Assignee: nobody → mcaceres
Flags: needinfo?(mcaceres)
Attached patch Use message instead (obsolete) — Splinter Review
Remove the CPOW, use frame script + message instead.
Attachment #8791110 - Flags: review?(mconley)
Comment on attachment 8791110 [details] [diff] [review]
Use message instead

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

Thanks Marcos! I have a few minor suggestions - see below!

::: testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js
@@ +6,5 @@
> +  sendAsyncMessage("get.ready.state.of.document", content.document.readyState);
> +`;
> +
> +function isDOMLoaded(browser){
> +  const mm = browser.messageManager;

I think you should be able to use ContentTask here instead of hand-rolling your own message. Example:

function isDOMLoaded(browser) {
  return ContentTask.spawn(browser, null, function*() {
    Assert.equal(content.document.readyState, "complete",
                 "Browser should be loaded.");
  });
}

Then you should be able to do:

yield isDOMLoaded(browser);

without mucking about with the results of that function, since the function itself does the assertion check.

@@ +45,4 @@
>      for (b of browsers) BrowserTestUtils.browserLoaded(b)
>    ));
>    let expected = 'Expected all promised browsers to have loaded.';
> +  for(const browser of browsers){

Space after "for", and before {. Probably best to run ./mach eslint testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js as well, just in case we've missed other things in your changes (no need to change things outside of your changes).

@@ +45,5 @@
>      for (b of browsers) BrowserTestUtils.browserLoaded(b)
>    ));
>    let expected = 'Expected all promised browsers to have loaded.';
> +  for(const browser of browsers){
> +    Assert.ok(yield isDOMLoaded(browser), expected);

With the above change to isDOMLoaded, I suppose you can do:

yield isDOMLoaded(browser);

here instead.
Attachment #8791110 - Flags: review?(mconley) → review-
Attached patch Use Mike's suggestions (obsolete) — Splinter Review
Didn't know about the funky ContentTask thing... it's funny it also uses the hack of converting the code to a string and passing it to a frame script. Makes sense tho :)
Attachment #8791110 - Attachment is obsolete: true
Attachment #8791897 - Flags: review?(mconley)
Comment on attachment 8791897 [details] [diff] [review]
Use Mike's suggestions

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

LGTM! Thanks!
Attachment #8791897 - Flags: review?(mconley) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb62c341a44
Fix intermittent dead CPOW failure in browser_browserLoaded_content_loaded.js. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffb62c341a44
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I tried to uplift this to Aurora, but hit test failures :(

https://treeherder.mozilla.org/logviewer.html#?job_id=3582628&repo=mozilla-aurora#L11331

TEST-UNEXPECTED-FAIL | testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js | Exception thrown - at chrome://mochitests/content/browser/testing/mochitest/tests/browser/browser_browserLoaded_content_loaded.js:39 - SyntaxError: missing = in const declaration

Looks like we're getting burned by bug 1101653, which got fixed in 51 by bug 1263355. Any way to work around it on 50?
Flags: needinfo?(mcaceres)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> Looks like we're getting burned by bug 1101653, which got fixed in 51 by bug
> 1263355. Any way to work around it on 50?

I'll change it to a `var`.
Flags: needinfo?(mcaceres)
This will work in olde firefox now. Carrying over Mike's r+. 

RyanVM, I'm not sure what the process is for back-porting this patch tho. Can you just use the attached patch? Do I need to make a new bug? I'm not sure where to check it in, etc.
Attachment #8791897 - Attachment is obsolete: true
Attachment #8792389 - Flags: review?(ryanvm)
Comment on attachment 8792389 [details] [diff] [review]
Switch to using var, for olde ff

Looks fine, thanks for doing this. I'll land it next time I'm pushing test-only fixes to 50 :).
Attachment #8792389 - Flags: review?(ryanvm)
You need to log in before you can comment on or make changes to this bug.