Update Screenshots to version 10.12.0

RESOLVED FIXED

Status

()

Firefox
Screenshots
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: _6a68, Assigned: _6a68)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
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?
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months 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
Blocks: 1386333
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+

Comment 6

5 months ago
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

5 months 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

5 months 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

5 months 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c71133188e61
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED

Updated

5 months ago
status-firefox56: --- → affected
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+
(Assignee)

Updated

5 months ago
Blocks: 1387598

Comment 12

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/f6bfd0fdadbc
status-firefox56: affected → fixed
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?
status-firefox55: --- → affected
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

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/c429763f95c3
status-firefox55: affected → fixed

Updated

4 months ago
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.