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

RESOLVED FIXED in Firefox 56

Status

Testing
mozscreenshots
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: Ehsan, Assigned: MattN)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [test-only code which doesn't ship])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
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?)
Component: Screenshots → General
Product: Cloud Services → Firefox
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?
Blocks: 1169179
Flags: needinfo?(ehsan)
Whiteboard: [test-only code which doesn't ship]
(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.
Flags: needinfo?(ehsan)
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
See Also: → bug 1245364
(Reporter)

Comment 6

a year ago
(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!)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
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

a year ago
mozreview-review
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/?
Attachment #8877805 - Flags: review?(mh+mozilla) → review+
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.
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 15

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/5c95a2f3eee8
Don't install mozscreenshots in the mochitest extensions dir. r=glandium
See Also: bug 1245364
Duplicate of this bug: 1245364
Summary: mozscreenshots shouldn't load anything before opening and painting the first browser window → mozscreenshots should only be loaded for mochitest-browser-screenshots runs

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c95a2f3eee8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Component: General → mozscreenshots
Product: Firefox → Testing
Target Milestone: Firefox 56 → ---
Target Milestone: --- → mozilla56
Duplicate of this bug: 1246974
You need to log in before you can comment on or make changes to this bug.