Closed
Bug 1389786
Opened 7 years ago
Closed 7 years ago
Update Screenshots to version 10.12.0
Categories
(Firefox :: Screenshots, defect)
Firefox
Screenshots
Tracking
()
RESOLVED
FIXED
People
(Reporter: jhirsch, Assigned: jhirsch)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
kmag
:
review+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-release+
|
Details |
This export contains a fix for Talos shutdown crashes discussed in bug 1386333, specifically comment 17. Details: if the Screenshots bootstrap.js shutdown() method is called with APP_SHUTDOWN as the reason, Screenshots will immediately call shutdown() on its embedded webextension. Changelog: https://github.com/mozilla-services/screenshots/blob/firefox-export/CHANGELOG.md#version-10120
Comment 1•7 years ago
|
||
My only concern here is that we're fixing this in Screenshots code now, but what's to stop our next embedded WebExtension, whatever it happens to be, from hitting this same footgun? Is there a way we can make Firefox more robust here?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Got R+ from :kmag in IRC on Friday[1], wasn't sure if additional review was required. [1] http://logs.glob.uno/?c=mozilla%23screenshots&s=11+Aug+2017&e=11+Aug+2017#c20946
Updated•7 years ago
|
Assignee: nobody → jhirsch
Comment 4•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1) > My only concern here is that we're fixing this in Screenshots code now, but > what's to stop our next embedded WebExtension, whatever it happens to be, > from hitting this same footgun? Is there a way we can make Firefox more > robust here? Normal embedded extensions handle it correctly automatically. Screenshots does its own thing rather than using the normal embedded extensions framework.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8897064 [details] Bug 1389786 - Export Screenshots 10.12.0 to Firefox; https://reviewboard.mozilla.org/r/168350/#review173600
Attachment #8897064 -
Flags: review?(kmaglione+bmo) → review+
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c71133188e61 Export Screenshots 10.12.0 to Firefox; r=kmag
Assignee | ||
Comment 7•7 years ago
|
||
This patch resolves the Talos crashes on beta caused by the previous version of Screenshots (see bug 1387598). Here's a Try run that completed without Talos errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55126aeb8ac636b2eaa34a0eb463b1383361d551
Assignee | ||
Comment 8•7 years ago
|
||
> Normal embedded extensions handle it correctly automatically. Screenshots does its own thing rather than using the normal embedded extensions framework.
True, but note that Screenshots delays startup only because the regular webextension startup code path regressed Talos.
If this situation has changed, we would be very happy to remove the delayed startup code.
If the startup costs still cause Talos regressions, then any other system addon webextension will run into this same issue.
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8897064 [details] Bug 1389786 - Export Screenshots 10.12.0 to Firefox; Approval Request Comment [Feature/Bug causing the regression]: Enabling Screenshots by default causes Talos crashes on Windows (bug 1387598). This patch fixes that bug, enabling us to safely enable Screenshots by default. [User impact if declined]: QA hasn't observed user-visible impact due to this bug, but this patch avoids crashing the content process on shutdown when Screenshots is enabled by default. [Is this code covered by automated tests?]: This code affects Talos tests, but does not have unit tests. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No, only automated tests seem to be affected by the bug. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low risk [Why is the change risky/not risky?]: This is a very small diff that speeds up Screenshots shutdown when Firefox is shutting down. This patch fixes the Windows Talos crashes observed on Beta; see comment 7 for a link to a successful Try run. [String changes made/needed]: None
Attachment #8897064 -
Flags: approval-mozilla-beta?
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c71133188e61
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 11•7 years ago
|
||
Comment on attachment 8897064 [details] Bug 1389786 - Export Screenshots 10.12.0 to Firefox; Fix Windows Talos crashes. Beta56+. Should be in 54.0b4.
Attachment #8897064 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f6bfd0fdadbc
Comment 13•7 years ago
|
||
Comment on attachment 8897064 [details] Bug 1389786 - Export Screenshots 10.12.0 to Firefox; (copy paste from previous comment) Approval Request Comment [Feature/Bug causing the regression]: Enabling Screenshots by default causes Talos crashes on Windows (bug 1387598). This patch fixes that bug, enabling us to safely enable Screenshots by default. [User impact if declined]: QA hasn't observed user-visible impact due to this bug, but this patch avoids crashing the content process on shutdown when Screenshots is enabled by default. [Is this code covered by automated tests?]: This code affects Talos tests, but does not have unit tests. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No, only automated tests seem to be affected by the bug. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low risk [Why is the change risky/not risky?]: This is a very small diff that speeds up Screenshots shutdown when Firefox is shutting down. This patch fixes the Windows Talos crashes observed on Beta; see comment 7 for a link to a successful Try run. [String changes made/needed]: None
Attachment #8897064 -
Flags: approval-mozilla-release?
Updated•7 years ago
|
status-firefox55:
--- → affected
Comment 14•7 years ago
|
||
Comment on attachment 8897064 [details] Bug 1389786 - Export Screenshots 10.12.0 to Firefox; we're pushing this out of band as a system add-on, let's update the in-tree copy too for 55.0.3
Attachment #8897064 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/c429763f95c3
Updated•7 years ago
|
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•