1444973 - Remove browser-test-overlay.xul

Status: RESOLVED FIXED

(Testing :: Mochitest, enhancement)

Description

[Brendan Dahl [:bdahl]]

The overlay loads a few scripts, so hopefully we can just replace it by dynamically loading those scripts.

Currently used by:
 - mochitest boostrap
 - remote mochitests

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: Bug 1444973 - Remove browser-test-overlay.xul.

On desktop use sub script loader during bootstrap instead of the overlay.
On android use a component that listens for browser windows to be created
and then use a sub script loader.

Review commit: https://reviewboard.mozilla.org/r/229178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229178/

Comment 2

[(not currently active) Ted Mielczarek]

I'm passing this review over to ahal, who I think is a more suitable reviewer these days.

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

https://reviewboard.mozilla.org/r/229178/#review235076

Thanks, lgtm. In case you haven't already, I'd test this on all platforms/build types on try.

::: testing/mochitest/RemoteMochitestStartup.js:21
(Diff revision 1)
> +            if (win.location.href !== "chrome://browser/content/browser.xul") {
> +                return;
> +            }

If we hit this, then I think the task would timeout without errors due to the 'once' argument.

Could we at least log that something bad happened?

::: testing/mochitest/runtestsremote.py:207
(Diff revision 1)
>      def addChromeToProfile(self, options):
>          manifest = MochitestDesktop.addChromeToProfile(self, options)
>  
>          # Support Firefox (browser), SeaMonkey (navigator), and Webapp Runtime (webapp).
>          if options.flavor == 'chrome':
>              # append overlay to chrome.manifest

nit: please update this comment

Comment 4

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229178/diff/1-2/

Comment 5

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

https://reviewboard.mozilla.org/r/229178/#review235076

> If we hit this, then I think the task would timeout without errors due to the 'once' argument.
> 
> Could we at least log that something bad happened?

A "chrome-document-global-created" could be triggered by a XUL page that isn't browser.xul, though I'm not sure if any android tests actually open any other XUL windows.

Comment 6

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229178/diff/2-3/

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

https://reviewboard.mozilla.org/r/229178/#review235076

> A "chrome-document-global-created" could be triggered by a XUL page that isn't browser.xul, though I'm not sure if any android tests actually open any other XUL windows.

Good point, overlooked the parent observer. I think this can be dropped.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deb3484ed429
Remove browser-test-overlay.xul. r=ahal

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/deb3484ed429

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/005891e345a4
Backed out changeset deb3484ed429 for remoteautomation.py failures on Android  a=backout

Comment 11

[Raul Gurzau (:RaulG)]

Backed out changeset deb3484ed429 (bug 1444973) for remoteautomation.py failures on Android a=backout

Backout link: https://hg.mozilla.org/mozilla-central/rev/005891e345a4c1ecba4bb087c8717fe00b798f03

Initial push that landed on autoland: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=deb3484ed4299e580f8d5c91e99c965149c9e88f

Push that has the revision above: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=e636edf00e6fbdc3206c9df4a1548ae38b3d13fa

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=169407456&repo=mozilla-central&lineNumber=1884

Log snippet:  
[task 2018-03-21T11:02:43.472Z] 11:02:43     INFO -  TEST-START | dom/bindings/test/test_bug1123516_maplikesetlikechrome.xul
[task 2018-03-21T11:02:43.472Z] 11:02:43     INFO -  TEST-SKIP | dom/bindings/test/test_bug1123516_maplikesetlikechrome.xul | took 1ms
[task 2018-03-21T11:02:43.472Z] 11:02:43     INFO -  Running manifest: devtools/server/socket/tests/chrome.ini
[task 2018-03-21T11:02:45.552Z] 11:02:45     INFO -  pushing /builds/worker/workspace/build/tests/mochitest/ to /mnt/sdcard/tests/chrome on device...
[task 2018-03-21T11:04:13.576Z] 11:04:13     INFO -  runtests.py | Failed to copy /builds/worker/workspace/build/tests/mochitest/hyphenation to profile
[task 2018-03-21T11:04:13.892Z] 11:04:13     INFO -  pk12util: PKCS12 IMPORT SUCCESSFUL
[task 2018-03-21T11:04:13.920Z] 11:04:13     INFO -  MochitestServer : launching [u'/builds/worker/workspace/build/hostutils/host-utils-60.0a2.en-US.linux-x86_64/xpcshell', '-g', '/builds/worker/workspace/build/hostutils/host-utils-60.0a2.en-US.linux-x86_64', '-f', '/builds/worker/workspace/build/hostutils/host-utils-60.0a2.en-US.linux-x86_64/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpCbCd5j.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '10.0.2.2'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/builds/worker/workspace/build/tests/mochitest/server.js']
[task 2018-03-21T11:04:13.920Z] 11:04:13     INFO -  runtests.py | Server pid: 987
[task 2018-03-21T11:04:13.925Z] 11:04:13     INFO -  runtests.py | Websocket server pid: 990
[task 2018-03-21T11:04:13.937Z] 11:04:13     INFO -  runtests.py | SSL tunnel pid: 993
[task 2018-03-21T11:04:18.981Z] 11:04:18     INFO -  runtests.py | Running with e10s: False
[task 2018-03-21T11:04:18.981Z] 11:04:18     INFO -  runtests.py | Running tests: start.
[task 2018-03-21T11:04:26.126Z] 11:04:26     INFO -  INFO | automation.py | Application pid: 1976
[task 2018-03-21T11:11:17.428Z] 11:11:17     INFO -  Browser unexpectedly found running. Killing...
[task 2018-03-21T11:11:17.453Z] 11:11:17     INFO -  TEST-INFO | started process screentopng
[task 2018-03-21T11:11:17.787Z] 11:11:17     INFO -  TEST-INFO | screentopng: exit 0
[task 2018-03-21T11:11:21.882Z] 11:11:21     INFO -  org.mozilla.fennec_aurora still alive after SIGABRT: waiting...
[task 2018-03-21T11:11:27.103Z] 11:11:27  WARNING -  TEST-UNEXPECTED-FAIL | remoteautomation.py | application timed out after 370 seconds with no output
[task 2018-03-21T11:11:27.104Z] 11:11:27     INFO -  INFO | automation.py | Application ran for: 0:07:08.121941
[task 2018-03-21T11:11:27.104Z] 11:11:27     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpUq6Mvspidlog
[task 2018-03-21T11:11:27.272Z] 11:11:27     INFO -  /data/anr/traces.txt not found
[task 2018-03-21T11:11:27.441Z] 11:11:27     INFO -  /data/tombstones does not exist; tombstone check skipped
[task 2018-03-21T11:11:28.302Z] 11:11:28     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/bUaSyr8JTveFPQqspLUYQw/artifacts/public/build/target.crashreporter-symbols.zip
[task 2018-03-21T11:11:32.330Z] 11:11:32     INFO -  mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmptVeYdO/6ffc52d7-bdb4-4667-ea49-f84860611551.dmp /tmp/tmpTsALDn
[task 2018-03-21T11:11:39.765Z] 11:11:39     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/6ffc52d7-bdb4-4667-ea49-f84860611551.dmp
[task 2018-03-21T11:11:39.765Z] 11:11:39     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/6ffc52d7-bdb4-4667-ea49-f84860611551.extra
[task 2018-03-21T11:11:39.768Z] 11:11:39  WARNING -  PROCESS-CRASH | remoteautomation.py | application crashed [@ libc.so + 0x26837]
[task 2018-03-21T11:11:39.769Z] 11:11:39     INFO -  Crash dump filename: /tmp/tmptVeYdO/6ffc52d7-bdb4-4667-ea49-f84860611551.dmp

Comment 12

[Brendan Dahl [:bdahl]]

Strange, I wasn't seeing any failures on try for android.

Comment 13

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229178/diff/3-4/

Comment 14

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229178/diff/4-5/

Comment 15

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Looking better now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f462cc66f7ef53591a0f6ef284f8175cfce506ba

I don't love the way of using a manifest comment to signal to the browser that we need to load the test files, but I don't really have any better ideas.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

https://reviewboard.mozilla.org/r/229178/#review236086

::: testing/mochitest/bootstrap.js:49
(Diff revision 5)
>    return new TextDecoder().decode(buffer);
>  }
>  
>  function androidStartup(data, reason) {
> +  // Only browser chrome tests need help starting.
> +  if (readSync(data.resourceURI.resolve("chrome.manifest")).includes('MOCHITEST_CHROME')) {

I think you could check that the pref "mochitest.testRoot" ends with the string "/chrome". Alternatively, we could create a new "mochitest.flavor" pref around here:
https://searchfox.org/mozilla-central/source/testing/mochitest/runtests.py#1847

(set it to the value of options.flavor, which should be 'chrome')

::: testing/mochitest/runtestsremote.py:206
(Diff revision 5)
> -            # append overlay to chrome.manifest
> -            chrome = ("overlay chrome://browser/content/browser.xul "
> -                      "chrome://mochikit/content/browser-test-overlay.xul")
> +            # append comment to chrome.manifest to signal boostrap.js to start tests
> +            chrome = """
> +# MOCHITEST_CHROME

With the above recommendation fixed you can remove this whole function.

Comment 17

[Brendan Dahl [:bdahl]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229178/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adedd212cd72
Remove browser-test-overlay.xul. r=ahal

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/adedd212cd72

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8960402 [details]
Bug 1444973 - Remove browser-test-overlay.xul.

https://reviewboard.mozilla.org/r/229178/#review236830

::: testing/mochitest/runtestsremote.py
(Diff revision 6)
> -    def addChromeToProfile(self, options):
> -        manifest = MochitestDesktop.addChromeToProfile(self, options)
> -
> -        # Support Firefox (browser), SeaMonkey (navigator), and Webapp Runtime (webapp).
> -        if options.flavor == 'chrome':
> -            # append overlay to chrome.manifest
> -            chrome = ("overlay chrome://browser/content/browser.xul "
> -                      "chrome://mochikit/content/browser-test-overlay.xul")
> -            path = os.path.join(options.profilePath, 'extensions', 'staged',
> -                                '[e-mail]', 'chrome.manifest')
> -            with open(path, "a") as f:
> -                f.write(chrome)
> -        return manifest

You have no idea how happy this makes me.