Closed Bug 1290980 Opened 4 years ago Closed 4 years ago

Intermittent dom/manifest/test/browser_fire_install_event.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: marcosc)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
This is one of the more common dead CPOW oranges being hit in automation. Can you please take a look, Marcos?

TEST-UNEXPECTED-FAIL | dom/manifest/test/browser_fire_install_event.js | Uncaught exception - at resource://gre/modules/RemoteAddonsParent.jsm:880 - Error: operation not possible on dead CPOW
Stack trace:
    getContentDocument@resource://gre/modules/RemoteAddonsParent.jsm:880:3
    RemoteBrowserElementInterposition.getters.contentDocument@resource://gre/modules/RemoteAddonsParent.jsm:897:10
    AddonInterpositionService.prototype.interposeProperty/desc.get@resource://gre/components/multiprocessShims.js:165:38
    theTest/responsePromise<@chrome://mochitests/content/browser/dom/manifest/test/browser_fire_install_event.js:26:5
    theTest@chrome://mochitests/content/browser/dom/manifest/test/browser_fire_install_event.js:25:27
    @chrome://mochitests/content/browser/dom/manifest/test/browser_fire_install_event.js:43:9
    @chrome://mochitests/content/browser/dom/manifest/test/browser_fire_install_event.js:43:9
    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:741:59
    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:741:59

Looks like this is coming from line 26 maybe?
> aBrowser.contentDocument.addEventListener("dom.manifest.oninstall", resolve);
Flags: needinfo?(mcaceres)
Assignee: nobody → mcaceres
Flags: needinfo?(mcaceres)
Attached patch Use message instead (obsolete) — Splinter Review
Mike, wdyt? I saw other tests doing this, um, "hack" with frame scripts as data URLs. Is that ok?
Attachment #8791066 - Flags: review?(mconley)
Comment on attachment 8791066 [details] [diff] [review]
Use message instead

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

Thanks again, Marcos! Glad to see us de-CPOW'ing our tests. Thankfully, I think there's a simpler mechanism we can use for messaging between the parent and content. See below.

::: dom/manifest/test/browser_fire_install_event.js
@@ +31,2 @@
>    // Resolves when we get a custom event with the correct name
> +  mm.loadFrameScript("data:," + frameScript, true);

This is another place where ContentTask.spawn is useful.

I believe you can do:

const responsePromise = ContentTask.spawn(aBrowser, null, function*() {
  return new Promise((resolve) => {
    addEventListener("install", function onInstall() {
      removeEventListener("install", onInstall);
      resolve();
    });
  });
});

and that way, you don't need to hook up your own framescript or do your own messaging - ContentTask will take care of it for you.
Attachment #8791066 - Flags: review?(mconley) → review-
Attached patch Use Mike's suggestions (obsolete) — Splinter Review
Awesome suggestions, thanks Mike!
Attachment #8791066 - Attachment is obsolete: true
Attachment #8791895 - Flags: review?(mconley)
Comment on attachment 8791895 [details] [diff] [review]
Use Mike's suggestions

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

This looks mostly fine - just clearing review request until I hear more about this event name change.

::: dom/manifest/test/browser_fire_install_event.js
@@ +21,5 @@
>  // This cause file_reg_install_event.html to be dynamically change.
>  function* theTest(aBrowser) {
>    aBrowser.allowEvents = true;
> +  let waitForInstall = ContentTask.spawn(aBrowser, null, function*() {
> +    yield ContentTaskUtils.waitForEvent(content.window, "install");

Did this event name change? It was "dom.manifest.oninstall" before, and it's "install" now. Are they synonymous?

@@ +30,5 @@
> +  try {
> +    yield waitForInstall;
> +    ok(true, "Install event fired");
> +  } catch (err) {
> +    ok(false, "Install event didn't fired: " + err.message);

Typo: "fired" -> "fire"
Attachment #8791895 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #7)
> Comment on attachment 8791895 [details] [diff] [review]
> Use Mike's suggestions
> 
> Review of attachment 8791895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly fine - just clearing review request until I hear more
> about this event name change.
> 
> ::: dom/manifest/test/browser_fire_install_event.js
> @@ +21,5 @@
> >  // This cause file_reg_install_event.html to be dynamically change.
> >  function* theTest(aBrowser) {
> >    aBrowser.allowEvents = true;
> > +  let waitForInstall = ContentTask.spawn(aBrowser, null, function*() {
> > +    yield ContentTaskUtils.waitForEvent(content.window, "install");
> 
> Did this event name change? It was "dom.manifest.oninstall" before, and it's
> "install" now. Are they synonymous?

Sorry about the confusion. The "dom.manifest.oninstall" was the message name that triggers the event to fire in the frame script, not the event. The event is "oninstall" on the window object. 

It is defined here:
https://www.w3.org/TR/appmanifest/#oninstall-attribute

 
> @@ +30,5 @@
> > +  try {
> > +    yield waitForInstall;
> > +    ok(true, "Install event fired");
> > +  } catch (err) {
> > +    ok(false, "Install event didn't fired: " + err.message);
> 
> Typo: "fired" -> "fire"

D'oh. Fixed!
Attached patch Fix typoSplinter Review
Attachment #8791895 - Attachment is obsolete: true
Attachment #8792386 - Flags: review?(mconley)
Comment on attachment 8792386 [details] [diff] [review]
Fix typo

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

Okie dokie, let's do it. Thanks MarcosC!
Attachment #8792386 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/073e1d56b6cd
Fix intermittent e10s issue with oninstall. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/073e1d56b6cd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.