Closed Bug 1253655 Opened 8 years ago Closed 8 years ago

Intermittent browser_inspector_gcli-inspect-command.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: RyanVM, Assigned: pbro)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [btpp-backlog])

Attachments

(1 file)

Bug 1253655 - Get rid of CPOWs in some commandline tests;
58 bytes, text/x-review-board-request
jsnajdr
: review+
Details 
https://treeherder.mozilla.org/logviewer.html#?job_id=17598226&repo=try

 21:41:48     INFO -  7983 INFO TEST-START | devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js
 21:41:49     INFO -  TEST-INFO | started process screentopng
 21:41:51     INFO -  TEST-INFO | screentopng: exit 0
 21:41:51     INFO -  7984 INFO checking window state
 21:41:51     INFO -  7985 INFO Entering test bound
 21:41:51     INFO -  7986 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW
 21:41:51     INFO -  7987 INFO Leaving test bound
 21:41:51     INFO -  MEMORY STAT | vsize 1207MB | residentFast 314MB | heapAllocated 133MB
21:41:51 INFO - 7988 INFO TEST-OK | devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js | took 1557ms
Filter on CLIMBING SHOES
Priority: -- → P3
Whiteboard: [btpp-backlog]
Inspector bug triage (filter on CLIMBING SHOES).
Priority: P3 → P2
14:22:51     INFO -  323 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js | Uncaught exception - at resource://gre/modules/RemoteAddonsParent.jsm:880 - Error: operation not possible on dead CPOW
14:22:51     INFO -  Stack trace:
14:22:51     INFO -      getContentDocument@resource://gre/modules/RemoteAddonsParent.jsm:880:3
14:22:51     INFO -      RemoteBrowserElementInterposition.getters.contentDocument@resource://gre/modules/RemoteAddonsParent.jsm:897:10
14:22:51     INFO -      AddonInterpositionService.prototype.interposeProperty/desc.get@resource://gre/components/multiprocessShims.js:165:38
14:22:51     INFO -      helpers.addTab/loaded<@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:136:7
14:22:51     INFO -      Async*helpers.addTab@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:135:18
14:22:51     INFO -      helpers.addTabWithToolbar@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:339:12
14:22:51     INFO -      @chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js:12:10
14:22:51     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
14:22:51     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
14:22:51     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
This is currently the thirdmost frequent dead CPOW orange in automation. Can you take a look when you get a chance, Patrick?

TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_gcli-inspect-command.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
    helpers.addTab/loaded<@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:136:7
    Async*helpers.addTab@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:135:18
    helpers.addTabWithToolbar@chrome://mochitests/content/browser/devtools/client/commandline/test/helpers.js:339:12
    @chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_gcli-inspect-command.js:12:10
    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
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
Flags: needinfo?(pbrosset)
Taking this bug. The CPOWs are pretty obvious in this test suite. Removing them will take some work, but shouldn't be too hard.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Comment on attachment 8791962 [details]
Bug 1253655 - Get rid of CPOWs in some commandline tests;

https://reviewboard.mozilla.org/r/79236/#review77804

Changes in this patch look good. I'm only pointing out a few usages of ContentTask where the code is correct, but could be made more elegant and less verbose. Also, the doc comments should be updated at a few places.

However, there are still few remaining CPOW accesses in the GCLI test suite:
- at two places in [1], there are accesses to gBrowser.contentWindow.applicationCache.
- in browser_gcli_js.js and mockCommands.js, there are accesses to the environment.window object, which is a contentWindow CPOW (see [2]).

Are there any recorded failures (reported bugs) at these places?

[1] http://searchfox.org/mozilla-central/search?q=contentWindow.applicationCache
[2] http://searchfox.org/mozilla-central/source/devtools/client/shared/developer-toolbar.js#204

::: devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js:50
(Diff revision 1)
> -  is(options.window.location.href, PAGE_1, "page 1 loaded");
>  
> +  let href = yield ContentTask.spawn(options.browser, {}, function* () {
> +    return content.location.href;
> +  });
> +  is(href, PAGE_1, "page 1 loaded");

You don't need to spawn a ContentTask just to get the URL. options.browser.currentURI.spec is equally good.

Use for example here:
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_page-nav.js#30

::: devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js:58
(Diff revision 1)
> -  is(options.window.location.href, PAGE_3, "page 3 loaded");
>  
> +  href = yield ContentTask.spawn(options.browser, {}, function* () {
> +    return content.location.href;
> +  });
> +  is(href, PAGE_3, "page 3 loaded");

Ditto.

::: devtools/client/commandline/test/browser_cmd_media.js:50
(Diff revision 1)
>          },
> -        post: function () {
> -          let body = options.window.document.body;
> -          let style = options.window.getComputedStyle(body);
> -          is(style.backgroundColor, "rgb(255, 255, 0)", "media correctly emulated");
> -        }
> +        post: Task.async(function* () {
> +          let color = yield ContentTask.spawn(options.browser, {}, function* () {
> +            return content.getComputedStyle(content.document.body).backgroundColor;
> +          });
> +          is(color, "rgb(255, 255, 0)", "media correctly emulated");

You can use the common assertion functions (is, ok, todo, info) directly inside the content task function:

yield ContentTask.spawn(browser, null, function* () {
  ok(something, "something in content");
});

ContentTask has support for sending the assertion results back to the parent process and reporting them there.

::: devtools/client/commandline/test/browser_cmd_media.js:72
(Diff revision 1)
> -        post: function () {
> -          let body = options.window.document.body;
> -          let style = options.window.getComputedStyle(body);
> -          is(style.backgroundColor, "rgb(255, 255, 255)", "media reset");
> -        }
> +        post: Task.async(function* () {
> +          let color = yield ContentTask.spawn(options.browser, {}, function* () {
> +            return content.getComputedStyle(content.document.body).backgroundColor;
> +          });
> +          is(color, "rgb(255, 255, 255)", "media reset");
> +        })

Ditto.

::: devtools/client/commandline/test/browser_cmd_pagemod_export.js:62
(Diff revision 1)
> +        });
> +
>          isnot(openURL.indexOf('<html lang="en">'), -1, "export html works: <html>");
>          isnot(openURL.indexOf("<title>GCLI"), -1, "export html works: <title>");
>          isnot(openURL.indexOf('<p id="someid">#'), -1, "export html works: <p>");
> -      }
> +      })

The isnot's could be inside the ContentTask.

::: devtools/client/commandline/test/browser_cmd_pagemod_export.js:89
(Diff revision 1)
>  
> -  options.window.open = oldOpen;
> -  oldOpen = undefined;
> +  yield ContentTask.spawn(options.browser, {}, function* () {
> +    content.wrappedJSObject.open = content.wrappedJSObject.oldOpen;
> +    content.wrappedJSObject.openURL = undefined;
> +    content.wrappedJSObject.oldOpen = undefined;
> +  });

nit: delete content.wrappedJSObject.oldOpen?

::: devtools/client/commandline/test/helpers.js:111
(Diff revision 1)
>   * - chromeWindow: XUL window parent of created tab. a.k.a 'window' in mochitest
>   * - tab: The new XUL tab element, as returned by gBrowser.addTab()
>   * - target: The debug target as defined by the devtools framework
>   * - browser: The XUL browser element for the given tab
>   * - window: Content window for the created tab. a.k.a 'content' in mochitest
>   * - isFirefox: Always true. Allows test sharing with GCLI

Change the doc comment to reflect that "window" is no longer returned.

::: devtools/client/commandline/test/helpers.js:167
(Diff revision 1)
>   * - tab
>   * - browser
>   * - target
>   * - document
>   * - window
>   * @return A promise which resolves to the options object when the 'load' event

Change the doc comment to reflect that "window" and "document" are no longer returned.

::: devtools/client/commandline/test/helpers.js:240
(Diff revision 1)
>  
> -    var promise = helpers.listenOnce(options.browser, "load", true).then(function () {
> -      options.document = options.browser.contentDocument;
> -      options.window = options.document.defaultView;
> -      return options;
> +    var onLoaded = helpers.listenOnce(options.browser, "load", true);
> +
> +    yield ContentTask.spawn(options.browser, url, function* (newUrl) {
> +      content.document.location.href = newUrl;
>      });

ContentTask is not needed to load a URL - use:

options.browser.loadURI(newUrl);
Attachment #8791962 - Flags: review?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #18)
> However, there are still few remaining CPOW accesses in the GCLI test suite:
> - at two places in [1], there are accesses to
> gBrowser.contentWindow.applicationCache.
> - in browser_gcli_js.js and mockCommands.js, there are accesses to the
> environment.window object, which is a contentWindow CPOW (see [2]).
> 
> Are there any recorded failures (reported bugs) at these places?
> 
> [1]
> http://searchfox.org/mozilla-central/search?q=contentWindow.applicationCache
> [2]
> http://searchfox.org/mozilla-central/source/devtools/client/shared/developer-
> toolbar.js#204
My goal here was to only fix browser_inspector_gcli-inspect-command.js which belongs in the inspector test folder, but in order to do this, I ended up changing more things inside the gcli test folder. 
Ideally, I'd like to keep away from changing too much there, because gcli tests haven't even been made e10s-compliant (the whole test suite is skipped for now), and would really like to fix this one CPOW usage quick (since it's frequent) rather than go down the rabbit hole of gcli e10s test refactoring :)
I'm not currently in a position that gives me a lot of time for this.
So I'm going to suggest that we do this in other bugs.

> ::: devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js:50
> (Diff revision 1)
> > -  is(options.window.location.href, PAGE_1, "page 1 loaded");
> >  
> > +  let href = yield ContentTask.spawn(options.browser, {}, function* () {
> > +    return content.location.href;
> > +  });
> > +  is(href, PAGE_1, "page 1 loaded");
> 
> You don't need to spawn a ContentTask just to get the URL.
> options.browser.currentURI.spec is equally good.
> 
> Use for example here:
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/
> browser_net_page-nav.js#30
Cool, thanks. I had used this in the past but somehow forgot about it.

> ::: devtools/client/commandline/test/browser_cmd_media.js:50
> (Diff revision 1)
> >          },
> > -        post: function () {
> > -          let body = options.window.document.body;
> > -          let style = options.window.getComputedStyle(body);
> > -          is(style.backgroundColor, "rgb(255, 255, 0)", "media correctly emulated");
> > -        }
> > +        post: Task.async(function* () {
> > +          let color = yield ContentTask.spawn(options.browser, {}, function* () {
> > +            return content.getComputedStyle(content.document.body).backgroundColor;
> > +          });
> > +          is(color, "rgb(255, 255, 0)", "media correctly emulated");
> 
> You can use the common assertion functions (is, ok, todo, info) directly
> inside the content task function:
> 
> yield ContentTask.spawn(browser, null, function* () {
>   ok(something, "something in content");
> });
> 
> ContentTask has support for sending the assertion results back to the parent
> process and reporting them there.
Nice, I had no idea.
 
> ::: devtools/client/commandline/test/helpers.js:240
> (Diff revision 1)
> >  
> > -    var promise = helpers.listenOnce(options.browser, "load", true).then(function () {
> > -      options.document = options.browser.contentDocument;
> > -      options.window = options.document.defaultView;
> > -      return options;
> > +    var onLoaded = helpers.listenOnce(options.browser, "load", true);
> > +
> > +    yield ContentTask.spawn(options.browser, url, function* (newUrl) {
> > +      content.document.location.href = newUrl;
> >      });
> 
> ContentTask is not needed to load a URL - use:
> 
> options.browser.loadURI(newUrl);
Ah thanks!
Comment on attachment 8791962 [details]
Bug 1253655 - Get rid of CPOWs in some commandline tests;

https://reviewboard.mozilla.org/r/79236/#review78122

::: devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js:47
(Diff revisions 1 - 2)
> -  is(href, PAGE_1, "page 1 loaded");
> -
>    // Page 2 is a frame in page 1. JS in the page navigates to page 3.
>    yield helpers.listenOnce(options.browser, "load", true);
> +  is(options.browser.currentURI.spec, PAGE_3, "page 3 loaded");
>  

There could still be a potential race there. The "browser" fires 3 load events:

1. PAGE_1 (main document)
2. PAGE_2 (iframe load)
3. PAGE_3 (main document, navigated after timeout)

Now, is there guaranteed ordering for events #1 and #2? Does the main document load fire only after all subframes are loaded? Or is it the other way and subframe load will start only after the parent frame is loaded? Or is everything loaded in parallel and the ordering is random?

helpers.navigate will wait using BrowserTestUtils.browserLoaded before it returns. This function subscribes to the "load" events and processes them until it finds the one from main document.

If the iframe event comes first, it will be discarded. If it comes second, it will be caught by the subsequent helpers.listenOnce, which was supposed to wait for PAGE_3, not PAGE_2. The assert that checks if currentURI == PAGE_3 will fail.

Possible solution:
Don't use helpers.navigate and check the events manually.

options.browser.loadURI(PAGE_1);
yield Promise.all([
  BrowserTestUtils.browserLoaded(options.browser, false, PAGE_1); // wait for main doc
  BrowserTestUtils.browserLoaded(options.browser, true, PAGE_2); // wait for the iframe
]);
is(options.browser.currentURI.spec, PAGE_1);

// now we're guaranteed to wait for PAGE_3
yield BrowserTestUtils.browserLoaded(options.browser, false, PAGE_3);
is(options.browser.currentURI.spec, PAGE_3);
Attachment #8791962 - Flags: review?(jsnajdr)
(In reply to Patrick Brosset <:pbro> from comment #20)
> My goal here was to only fix browser_inspector_gcli-inspect-command.js which
> belongs in the inspector test folder, but in order to do this, I ended up
> changing more things inside the gcli test folder. 
> Ideally, I'd like to keep away from changing too much there, because gcli
> tests haven't even been made e10s-compliant (the whole test suite is skipped
> for now), and would really like to fix this one CPOW usage quick (since it's
> frequent) rather than go down the rabbit hole of gcli e10s test refactoring
> :)
> I'm not currently in a position that gives me a lot of time for this.
> So I'm going to suggest that we do this in other bugs.

OK, comment 16 suggested that your ambition is to remove all CPOWs from the GCLI test suite, so I pointed out there are still some remaining ones :) No problem postponing them into another bug.
(In reply to Jarda Snajdr [:jsnajdr] from comment #22)
> Possible solution:
> Don't use helpers.navigate and check the events manually.
> 
> options.browser.loadURI(PAGE_1);
> yield Promise.all([
>   BrowserTestUtils.browserLoaded(options.browser, false, PAGE_1); // wait
> for main doc
>   BrowserTestUtils.browserLoaded(options.browser, true, PAGE_2); // wait for
> the iframe
> ]);
> is(options.browser.currentURI.spec, PAGE_1);
> 
> // now we're guaranteed to wait for PAGE_3
> yield BrowserTestUtils.browserLoaded(options.browser, false, PAGE_3);
> is(options.browser.currentURI.spec, PAGE_3);
Works for me. Thanks a lot for looking into this so thoroughly.
Comment on attachment 8791962 [details]
Bug 1253655 - Get rid of CPOWs in some commandline tests;

https://reviewboard.mozilla.org/r/79236/#review78352

r+ if everything is really green now.
Attachment #8791962 - Flags: review?(jsnajdr) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8a1efb73e742
Get rid of CPOWs in some commandline tests; r=jsnajdr
https://hg.mozilla.org/mozilla-central/rev/8a1efb73e742
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Blocks: 1269287
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: