Closed Bug 1030347 Opened 6 years ago Closed 5 years ago

Enable devtools/sourceeditor tests with e10s

Categories

(DevTools :: Source Editor, defect)

defect
Not set

Tracking

(e10s+)

RESOLVED FIXED
Firefox 34
Tracking Status
e10s + ---

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Whiteboard: [status:landedon])

Attachments

(1 file, 1 obsolete file)

The first time I ran the suite with --e10s I go the following error after accessing browser.contentWindow.wrappedJSObject: TypeError: win is undefined.

At this line http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_vimemacs.js#21

Oddly, it worked fine each run after that, so I'm not sure if that was an unrelated issue.
Running this:

mach mochitest-devtools --e10s browser/devtools/sourceeditor/

I am frequently running into the same error in a variety of the tests.  This is the pattern that is intermittently failing:

function test() {
  let tab = gBrowser.addTab();
  gBrowser.selectedTab = tab;

  let browser = gBrowser.getBrowserForTab(tab);
  browser.loadURI(URI);

  function check() {
    var win = browser.contentWindow.wrappedJSObject;
    var doc = win.document; // <- error
  }

  setTimeout(check, 100);
}
Attached patch e10s-sourceeditor.patch (obsolete) — Splinter Review
As discussed on #devtools, this seems to resolve the intermittent issues in the sourceeditor tests when running with e10s.  Does this look reasonable?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8446257 - Flags: feedback?(tomica+amo)
Comment on attachment 8446257 [details] [diff] [review]
e10s-sourceeditor.patch

so with e10s, the contentWindow is not an object in the same process anymore, but a CPOW (Cross Process Object Wrapper). and before the window is created in the child process, it's undefined.

the "proper way" to do this would to move most of the logic that touches content to the frame script, and report the result via a message, but this also works, especially for test code.
Attachment #8446257 - Flags: feedback?(tomica+amo) → feedback+
Whiteboard: [status:planned]
This was a little tricky because the test harness was interacting directly with the CodeMirror test harness to pass assertions along from the content frame.  

This enables the two tests that were not working with e10s.  I've confirmed that failures are being reported locally by purposefully adding a bad eq() call in one of the CM tests, like cm_multi_test.js and then running `mach mochitest-devtools --e10s browser/devtools/sourceeditor/test/browser_codemirror.js`
Attachment #8446257 - Attachment is obsolete: true
Attachment #8467953 - Flags: review?(jwalker)
Comment on attachment 8467953 [details] [diff] [review]
e10s-sourceeditor.patch

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

::: browser/devtools/sourceeditor/test/head.js
@@ +66,5 @@
> + */
> +function loadHelperScript(filePath) {
> +  let testDir = gTestPath.substr(0, gTestPath.lastIndexOf("/"));
> +  Services.scriptloader.loadSubScript(testDir + "/" + filePath, this);
> +}

I wish we had a head.js for the whole of devtools...
Attachment #8467953 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #5)
> Comment on attachment 8467953 [details] [diff] [review]
> e10s-sourceeditor.patch
> 
> Review of attachment 8467953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/sourceeditor/test/head.js
> @@ +66,5 @@
> > + */
> > +function loadHelperScript(filePath) {
> > +  let testDir = gTestPath.substr(0, gTestPath.lastIndexOf("/"));
> > +  Services.scriptloader.loadSubScript(testDir + "/" + filePath, this);
> > +}
> 
> I wish we had a head.js for the whole of devtools...

Yes, save a ton of duplication.  Do you think it'd be possible to do?
Whiteboard: [status:planned] → [status:inflight]
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7255d7474455

(I pushed this to inbound as a ride-along because I didn't have anything to push to fx-team at the moment, in case you're wondering)
Keywords: checkin-needed
Whiteboard: [status:inflight]
https://hg.mozilla.org/mozilla-central/rev/7255d7474455
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Whiteboard: [status:landedon]
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.