1371339 - mozscreenshots should only be loaded for mochitest-browser-screenshots runs

Status: RESOLVED FIXED

(Testing :: mozscreenshots, defect)

Description

[(no longer active)]

browser/base/content/test/performance/browser_startup.js indicates that the following scripts are loaded before opening the first window:

chrome://mozscreenshots/content/Screenshot.jsm
chrome://mozscreenshots/content/TestRunner.jsm

The second script shouldn't be loaded at any time even past startup since it seems to be only used for testing.

Comment 1

[Florian Quèze [:florian]]

It's from http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/tools/mozscreenshots/mozscreenshots/extension/bootstrap.js#38 so likely only when running under the test infrastructure (I hope so at least!)

Comment 2

[Ian Bicking (:ianbicking)]

This isn't the Firefox Screenshots code, it's something else (what exactly it is I'm not sure, maybe something related to making screenshots in automated tests?)

Comment 3

[Matthew N. [:MattN]]

Yeah, this is for https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots which is only for our own automation.

This code shouldn't be shipping with Firefox as it's only built with TEST_DIRS: https://dxr.mozilla.org/mozilla-central/rev/981da978f1f686ad024fa958c9d27d2f8acc5ad0/browser/moz.build#29-31

Shall we just add a comment in browser_startup.js and leave this alone?

Comment 4

[Florian Quèze [:florian]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)

> This code shouldn't be shipping with Firefox as it's only built with
> TEST_DIRS:
> https://dxr.mozilla.org/mozilla-central/rev/
> 981da978f1f686ad024fa958c9d27d2f8acc5ad0/browser/moz.build#29-31
> 
> Shall we just add a comment in browser_startup.js and leave this alone?

The problem with whitelisting test-only stuff in browser_startup.js is that it's likely to hide problems that are part of shipping code. The more we run test-only code during startup on our test infrastructure, the less we are able to write reliable startup tests.

How hard would it be to delay loading this code until startup is fully done?

If this code doesn't need to be running during startup, it should be just a matter of changing http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/browser/tools/mozscreenshots/mozscreenshots/extension/bootstrap.js#44 so that it runs later (ideally wait for the "sessionstore-windows-restored" notification and then for an idle callback). I think most system add-ons will need a similar treatment soon.

Comment 5

[Matthew N. [:MattN]]

In this case I think the easier fix is to not even install this extension outside of the M(ss) jobs. I already use installTemporaryAddon[1] to install the extension during the actual "test" so I think it's a matter of not having it auto-installed via TEST_EXTENSIONS_DIR[2]. That should also fix bug 1245364.

[1] https://dxr.mozilla.org/mozilla-central/rev/2a63a6c35033b5cbc6c98cabc7657c7290284691/browser/tools/mozscreenshots/head.js#20-23
[2] https://dxr.mozilla.org/mozilla-central/rev/2a63a6c35033b5cbc6c98cabc7657c7290284691/browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in#5

Comment 6

[(no longer active)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5)
> In this case I think the easier fix is to not even install this extension
> outside of the M(ss) jobs.

That would be a fantastic fix!

(Sorry for the confusion about "screenshots" by the way!)

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1371339 - Don't install mozscreenshots in the mochitest extensions dir.

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

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8877805 [details]
Bug 1371339 - Don't install mozscreenshots in the mochitest extensions dir.

https://reviewboard.mozilla.org/r/149224/#review153708

::: browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in:11
(Diff revision 1)
>  libs::
> -	(cd $(DIST)/xpi-stage && tar $(TAR_CREATE_FLAGS) - $(XPI_NAME)) | (cd $(TEST_EXTENSIONS_DIR) && tar -xf -)
> +	(cd $(DIST)/xpi-stage && tar $(TAR_CREATE_FLAGS) - $(XPI_NAME)) | (cd $(OUTPUT_DIR) && tar -xf -)

Ideally I'd just want to symlink all the extension files inside OUTPUT_DIR but I wasn't sure how to do that for test-only code and this at least solves the problems at hand I think.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8877805 [details]
Bug 1371339 - Don't install mozscreenshots in the mochitest extensions dir.

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

Comment 10

[Matthew N. [:MattN]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca362eb7f7464a57137e7b257776a44779dfadf1

Johann: note that this will cause some changes to the paths shown in the URL bar when it lands.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8877805 [details]
Bug 1371339 - Don't install mozscreenshots in the mochitest extensions dir.

https://reviewboard.mozilla.org/r/149224/#review154226

Why the double browser/?

Comment 12

[Mike Hommey [:glandium]]

Side note: "only built with TEST_DIRS" doesn't mean "doesn't ship with firefox". As long as it ends up in dist/bin, and it's in package-manifest, it's going to be shipped.

Comment 13

[Matthew N. [:MattN]]

Thanks

(In reply to Mike Hommey [:glandium] from comment #11)
> Comment on attachment 8877805 [details]
> Bug 1371339 - Don't install mozscreenshots in the mochitest extensions dir.
> 
> https://reviewboard.mozilla.org/r/149224/#review154226
> 
> Why the double browser/?

I believe the one comes from /browser/ (as opposed to toolkit) and the other comes from mochitest-browser since mozscreenshots runs as mochitest-browser-chrome

(In reply to Mike Hommey [:glandium] from comment #12)
> Side note: "only built with TEST_DIRS" doesn't mean "doesn't ship with
> firefox". As long as it ends up in dist/bin, and it's in package-manifest,
> it's going to be shipped.

OK, makes sense. In this case I only see mozscreenshots in obj-fx-opt/dist/xpi-stage/mozscreenshots, not dist/bin so I think we're good.

Comment 14

[Matthew N. [:MattN]]

From looking at the images I think this is good to land: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=da66c4a05fda49d457d9411a7092fed87cf9e53a&newProject=try&newRev=ca362eb7f7464a57137e7b257776a44779dfadf1

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c95a2f3eee8
Don't install mozscreenshots in the mochitest extensions dir. r=glandium

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c95a2f3eee8