1030347 - Enable devtools/sourceeditor tests with e10s

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

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);
}

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: e10s-sourceeditor.patch

As discussed on #devtools, this seems to resolve the intermittent issues in the sourceeditor tests when running with e10s.  Does this look reasonable?

Comment 3

[Tomislav Jovanovic :zombie]

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.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: e10s-sourceeditor.patch

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`

Comment 5

[Joe Walker [: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...

Comment 6

[Brian Grinstead [:bgrins]]

(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?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 8

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/7255d7474455