Closed Bug 1389786 Opened 2 years ago Closed 2 years ago

Update Screenshots to version 10.12.0

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- fixed
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: _6a68, Assigned: _6a68)

References

Details

Attachments

(1 file)

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
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?
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
Assignee: nobody → jhirsch
(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 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
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
> 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.
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?
https://hg.mozilla.org/mozilla-central/rev/c71133188e61
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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 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?
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+
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.