|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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.
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!)
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?
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.
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 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. That should also fix bug 1245364.  https://dxr.mozilla.org/mozilla-central/rev/2a63a6c35033b5cbc6c98cabc7657c7290284691/browser/tools/mozscreenshots/head.js#20-23  https://dxr.mozilla.org/mozilla-central/rev/2a63a6c35033b5cbc6c98cabc7657c7290284691/browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in#5
See Also: → bug 1245364
(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!)
a year ago
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
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.
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 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.
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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/5c95a2f3eee8 Don't install mozscreenshots in the mochitest extensions dir. r=glandium
a year ago
See Also: bug 1245364 →
Duplicate of this bug: 1245364
a year ago
Summary: mozscreenshots shouldn't load anything before opening and painting the first browser window → mozscreenshots should only be loaded for mochitest-browser-screenshots runs
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
11 months ago
Component: General → mozscreenshots
Product: Firefox → Testing
Target Milestone: Firefox 56 → ---
9 months ago
Duplicate of this bug: 1246974
You need to log in before you can comment on or make changes to this bug.