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

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: RyanVM, Assigned: pbro)

Tracking

(Blocks: 1 bug, {intermittent-failure})

unspecified
Unspecified
All
intermittent-failure

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 3

3 years ago
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]
tracking-e10s: --- → +
(Reporter)

Comment 4

3 years ago
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 hidden (Intermittent Failures Robot)
(Assignee)

Comment 8

3 years ago
Created 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 commit: https://reviewboard.mozilla.org/r/42979/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42979/
Attachment #8735852 - Flags: review?(poirot.alex)
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+
(Assignee)

Comment 10

3 years ago
(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)

Updated

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 14

2 years ago
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
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.