Closed Bug 1253935 Opened 8 years ago Closed 8 years ago

Frequent debug e10s browser_styleeditor_fetch-from-cache.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:65 - Error: cross-process JS call failed

Categories

(DevTools :: Style Editor, defect, P2)

Unspecified
All
defect

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: RyanVM, Assigned: pbro)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

We ran into this failure a lot over the weekend, and as best we can tell, it's tied to where in the chunk the test is run. In other words, we would push a change to the devtools manifests and this test would suddenly go permafail, then another change would make it go green again.

My best theory as to what might explain it is that while we use run-by-dir in these tests (launching a fresh Firefox instance between each directory), I *think* we recycle the profile between runs (because creating a new profile for each directory in run-by-dir mode caused a non-negligible runtime hit IIRC). So my suspicion is that this test somehow depends on state from other tests, which makes it susceptible to failures when those conditions aren't met due to chunking changes. At least, that's the most-plausible theory I've got :-)

Anyway, I'm disabling the test on Windows debug e10s for now to avoid noise in the trees (and other patches being backed out for inadvertently pushing this into permafail again).

https://treeherder.mozilla.org/logviewer.html#?job_id=23112616&repo=mozilla-inbound

20:38:04     INFO -  4 INFO TEST-START | devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js
20:38:04     INFO -  ++DOCSHELL 0E56C400 == 3 [pid = 1428] [id = 3]
20:38:04     INFO -  ++DOMWINDOW == 6 (0F5F5400) [pid = 1428] [serial = 6] [outer = 00000000]
20:38:04     INFO -  ++DOMWINDOW == 7 (0F5F8C00) [pid = 1428] [serial = 7] [outer = 0F5F5400]
20:38:04     INFO -  ++DOCSHELL 0E61A400 == 4 [pid = 1428] [id = 4]
20:38:04     INFO -  ++DOMWINDOW == 8 (0F5F6800) [pid = 1428] [serial = 8] [outer = 00000000]
20:38:04     INFO -  ++DOMWINDOW == 9 (0FA05800) [pid = 1428] [serial = 9] [outer = 0F5F6800]
20:38:04     INFO -  ++DOMWINDOW == 10 (0FA09800) [pid = 1428] [serial = 10] [outer = 0F5F6800]
20:38:04     INFO -  ++DOCSHELL 0F64B400 == 10 [pid = 2676] [id = 14]
20:38:04     INFO -  ++DOMWINDOW == 26 (0F64BC00) [pid = 2676] [serial = 33] [outer = 00000000]
20:38:04     INFO -  ++DOMWINDOW == 27 (0F652000) [pid = 2676] [serial = 34] [outer = 0F64BC00]
20:38:04     INFO -  ++DOMWINDOW == 28 (0F66D000) [pid = 2676] [serial = 35] [outer = 0F64BC00]
20:38:05     INFO -  ++DOCSHELL 0F864C00 == 11 [pid = 2676] [id = 15]
20:38:05     INFO -  ++DOMWINDOW == 29 (0F8AA800) [pid = 2676] [serial = 36] [outer = 00000000]
20:38:05     INFO -  ++DOMWINDOW == 30 (0F8B0400) [pid = 2676] [serial = 37] [outer = 0F8AA800]
20:38:05     INFO -  ++DOCSHELL 18365800 == 12 [pid = 2676] [id = 16]
20:38:05     INFO -  ++DOMWINDOW == 31 (1836CC00) [pid = 2676] [serial = 38] [outer = 00000000]
20:38:05     INFO -  ++DOMWINDOW == 32 (18A6FC00) [pid = 2676] [serial = 39] [outer = 1836CC00]
20:38:06     INFO -  TEST-INFO | started process screenshot
20:38:06     INFO -  TEST-INFO | screenshot: exit 0
20:38:06     INFO -  5 INFO checking window state
20:38:06     INFO -  6 INFO Entering test bound
20:38:06     INFO -  7 INFO Opening netmonitor
20:38:06     INFO -  8 INFO Adding a new tab with URL: 'about:blank'
20:38:06     INFO -  9 INFO URL 'about:blank' loading complete
20:38:06     INFO -  10 INFO Navigating to test page
20:38:06     INFO -  11 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:65 - Error: cross-process JS call failed
20:38:06     INFO -  Stack trace:
20:38:06     INFO -      navigateTo@chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:65:3
20:38:06     INFO -      @chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js:20:9
20:38:06     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:790:9
20:38:06     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:710:7
20:38:06     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
20:38:06     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:790:9
20:38:06     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:710:7
20:38:06     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
20:38:06     INFO -  12 INFO Leaving test bound
20:38:06     INFO -  [Child 1428] WARNING: site security information will not be persisted: file c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 260
20:38:06     INFO -  ++DOMWINDOW == 11 (0EF7EC00) [pid = 1428] [serial = 11] [outer = 0F5F6800]
20:38:06     INFO -  --DOCSHELL 10A87C00 == 11 [pid = 2676] [id = 11]
20:38:07     INFO -  --DOMWINDOW == 31 (10AD6000) [pid = 2676] [serial = 27] [outer = 00000000] [url = about:blank]
20:38:07     INFO -  --DOCSHELL 18365800 == 10 [pid = 2676] [id = 16]
20:38:07     INFO -  --DOMWINDOW == 30 (10A88800) [pid = 2676] [serial = 26] [outer = 00000000] [url = chrome://devtools/content/framework/toolbox.xul]
20:38:07     INFO -  --DOMWINDOW == 29 (1836CC00) [pid = 2676] [serial = 38] [outer = 00000000] [url = about:blank]
20:38:07     INFO -  --DOMWINDOW == 28 (18A70400) [pid = 2676] [serial = 29] [outer = 00000000] [url = chrome://devtools/content/styleeditor/styleeditor.xul]
20:38:07     INFO -  --DOMWINDOW == 27 (1C61D800) [pid = 2676] [serial = 31] [outer = 00000000] [url = data:text/html;charset=utf8,<!DOCTYPE%20html><html%20dir='ltr'>%20%20<head>%20%20%20%20<style>%20%20%20%20%20%20html,%20body%20{%20height:%20100%;%20}%20%20%20%20%20%20body%20{%20margin:%200;%20overflow:%20hidden;%20}%20%20%20%20%20%20.CodeMirror%20{%20width:%20100%;%20height:%20100%%20!important;%20line-height:%201.25%20!important;}%20%20%20%20</style><link%20rel='stylesheet'%20href='chrome://devtools/content/sourceeditor/codemirror/lib/codemirror.css'><link%20rel='stylesheet'%20href='chrome://devtools/content/sourceeditor/codemirror/addon/dialog/dialog.css'><link%20rel='stylesheet'%20href='chrome://devtools/content/sourceeditor/codemirror/mozilla.css'>%20%20</head>%20%20<body%20class='theme-body%20devtools-monospace'></body></html>]
20:38:07     INFO -  MEMORY STAT | vsize 632MB | vsizeMaxContiguous 541MB | residentFast 272MB | heapAllocated 71MB
20:38:07     INFO -  13 INFO TEST-OK | devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js | took 3012ms
I don't know why changing the way this test runs in chunks would affect it, but the error message is pretty telling:

> devtools/client/styleeditor/test/head.js:65 - Error: cross-process JS call failed

This line is `content.location = url` in this head.js helper:

function navigateTo(url) {
  let navigating = promise.defer();
  gBrowser.selectedBrowser.addEventListener("load", function onload() {
    gBrowser.selectedBrowser.removeEventListener("load", onload, true);
    navigating.resolve();
  }, true);
  content.location = url;
  return navigating.promise;
}

This is obviously a CPOW usage that should be cleaned up, and an easy fix too.
ContentTask.spawn can be used to navigate in the content process instead.

I suggest taking this opportunity import shared-head.js in this head.js to get rid of some of the code, and getting rid of all CPOW usages in these tests.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Happens on OSX too.
OS: Windows → All
Summary: Frequent Windows debug e10s browser_styleeditor_fetch-from-cache.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:65 - Error: cross-process JS call failed → Frequent debug e10s browser_styleeditor_fetch-from-cache.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:65 - Error: cross-process JS call failed
Comment on attachment 8735852 [details]
MozReview Request: Bug 1253935 - Remove all CPOW usages in styleeditor tests and use ContentTask instead of custom frame-script; r=ochameau

https://reviewboard.mozilla.org/r/42979/#review39483

Thanks for this cleanup. I'm happy to see another framescript being removed.
Before reenabling tests, have you run many try to verify it is no longer intermittent?

::: devtools/client/styleeditor/test/browser_styleeditor_transition_rule.js:27
(Diff revision 1)
>    editor.sourceEditor.setText(NEW_RULE);
>    editor.sourceEditor.setText(NEW_RULE + " ");
>  
>    yield styleChanges;
>  
> -  let rules = yield executeInContent("Test:cssRules", {
> +  let rules = yield ContentTask.spawn(tab.linkedBrowser, 0, function*(index) {

In browser_styleeditor_sync.js you are using gBrowser.selectedBrowser, here you are using tab.linkedBrowser. It would be great to be consistent.

::: devtools/client/styleeditor/test/head.js:62
(Diff revision 1)
>    }, true);
> -  content.location = url;
> -  return navigating.promise;
> -}
>  
> -function* cleanup() {
> +  yield ContentTask.spawn(browser, url, "u => content.location.href = u");

I would have done this instead of doing something in child:
  browser.loadURI(url);
  
(xul:browser code is then in charge of doing whatever is necessary to load this url)

Also don't we already have such helper elsewhere?

::: devtools/client/styleeditor/test/head.js:134
(Diff revision 1)
> -  return yield executeInContent("Test:GetComputedStylePropertyValue",
> -                                {selector,
> -                                pseudo,
> -                                name: propName});
> +  return yield ContentTask.spawn(gBrowser.selectedBrowser, args,
> +    `function*({selector, pseudo, name}) {
> +      let element = content.document.querySelector(selector);
> +      let style = content.getComputedStyle(element, pseudo);
> +      return style.getPropertyValue(name);
> +    }`

It would be easier to read without using a string here. Is there any reason to not use a regular function?
Attachment #8735852 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Thanks for this cleanup. I'm happy to see another framescript being removed.
> Before reenabling tests, have you run many try to verify it is no longer
> intermittent?
Yeah, this is still pending. Here's the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=546d7e3942f2&group_state=expanded

> In browser_styleeditor_sync.js you are using gBrowser.selectedBrowser, here
> you are using tab.linkedBrowser. It would be great to be consistent.
Ok, will do.

> ::: devtools/client/styleeditor/test/head.js:62
> > -function* cleanup() {
> > +  yield ContentTask.spawn(browser, url, "u => content.location.href = u");
> 
> I would have done this instead of doing something in child:
>   browser.loadURI(url);
>   
> (xul:browser code is then in charge of doing whatever is necessary to load
> this url)
Righ, looks better.
> Also don't we already have such helper elsewhere?
I'll look, but I'm not sure. We have bug 1181856 for using shared-head.js in styleeditor tests, but I'd like to avoid doing this big change in this patch.

> ::: devtools/client/styleeditor/test/head.js:134
> (Diff revision 1)
> > -  return yield executeInContent("Test:GetComputedStylePropertyValue",
> > -                                {selector,
> > -                                pseudo,
> > -                                name: propName});
> > +  return yield ContentTask.spawn(gBrowser.selectedBrowser, args,
> > +    `function*({selector, pseudo, name}) {
> > +      let element = content.document.querySelector(selector);
> > +      let style = content.getComputedStyle(element, pseudo);
> > +      return style.getPropertyValue(name);
> > +    }`
> 
> It would be easier to read without using a string here. Is there any reason
> to not use a regular function?
Yes, there was a reason when I did it, but I can't remember for sure. I'll try again.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8735852 [details]
MozReview Request: Bug 1253935 - Remove all CPOW usages in styleeditor tests and use ContentTask instead of custom frame-script; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42979/diff/1-2/
Attachment #8735852 - Attachment description: MozReview Request: Bug 1253935 - Remove all CPOW usages in styleeditor tests and use ContentTask instead of custom frame-script → MozReview Request: Bug 1253935 - Remove all CPOW usages in styleeditor tests and use ContentTask instead of custom frame-script; r=ochameau
Not sure why this is leave-open. Something landed on m-c in March and there has been no reports of test failures since. So I'm going to close it.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.