Closed
Bug 1386333
Opened 7 years ago
Closed 7 years ago
Remove Screenshots rollout pref
Categories
(Firefox :: Screenshots, defect)
Firefox
Screenshots
Tracking
()
RESOLVED
FIXED
Firefox 57
People
(Reporter: jhirsch, Assigned: jhirsch)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
standard8
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
We planned to use the `extensions.screenshots.system-disabled` pref to allow SHIELD to gradually roll out Screenshots to our Beta and Release populations. That's happening in 55, so we can now remove the `system-disabled` pref from Nightly.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhirsch
Assignee | ||
Updated•7 years ago
|
Summary: Remove Screenshots roll-out pref → Remove Screenshots rollout pref
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8892518 [details] Bug 1386333 - Remove Screenshots rollout pref; https://reviewboard.mozilla.org/r/163508/#review168882
Attachment #8892518 -
Flags: review?(standard8) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ba769a1d2f82 Remove Screenshots rollout pref; r=standard8
Keywords: checkin-needed
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba769a1d2f82
Comment 5•7 years ago
|
||
Comment on attachment 8892518 [details] Bug 1386333 - Remove Screenshots rollout pref; Approval Request Comment [Feature/Bug causing the regression]: It's not a regression. We'd like to drop this pref in beta to get us to 100% users w/ screenshots enabled [User impact if declined]: Beta users don't get the screenshots button. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes, screenshots is on in nightly. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: It's just removing a pref. Biggest risk is the screenshots service failing under too much traffic, but we were preffed on to 50% of beta already (via shield) and were fine and any problems would be fixable on the server. [String changes made/needed]: None
Attachment #8892518 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 6•7 years ago
|
||
Comment on attachment 8892518 [details] Bug 1386333 - Remove Screenshots rollout pref; This moves control of the rollout to the Shield infrastructure. Right now that means 100% of beta users will get Screenshots in 56 beta 2.
Attachment #8892518 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 7•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f1084f291de2
Comment 8•7 years ago
|
||
I had to back this out from Beta for making Win7 ts_paint nearly perma-crash. I confirmed this patch as the culprit with a series of Try pushes. https://hg.mozilla.org/releases/mozilla-beta/rev/57026202f82d The scary thing to me is that the stacks were all over the place. Trashed memory somehow? I worry that 57 will also be affected when it goes to Beta. Not sure if there's been any Try simulations yet during this cycle. https://treeherder.mozilla.org/logviewer.html#?job_id=122287926&repo=mozilla-beta https://treeherder.mozilla.org/logviewer.html#?job_id=122288473&repo=mozilla-beta https://treeherder.mozilla.org/logviewer.html#?job_id=122288491&repo=mozilla-beta https://treeherder.mozilla.org/logviewer.html#?job_id=122288496&repo=mozilla-beta https://treeherder.mozilla.org/logviewer.html#?job_id=122288500&repo=mozilla-beta https://treeherder.mozilla.org/logviewer.html#?job_id=122288504&repo=mozilla-beta
Comment 9•7 years ago
|
||
RyanVM, was this behavior observed when 56 was in Nightly?
Comment 10•7 years ago
|
||
I know the pref was disabled by default but I don't recall seeing this crash on our talos builds where we flipped the pref.
Comment 12•7 years ago
|
||
In IRC discussion with Jared, he's pointed out a few other crashes that look maybe related to this, but they've been difficult to narrow down. Beyond that, not as far as I know, but I also don't regularly watch Treeherder regularly, so I'm not a great person to answer questions about CI runs when 56 was on Nightly.
Flags: needinfo?(ryanvm)
Comment 13•7 years ago
|
||
These are shutdown crashes where we kill the content process because because it's still starting up when the parent process is shutting down. The shutdown blockers added in bug 1357486 were meant to prevent this, but it looks like the 1s timeout added in bug 1380267 is not quite enough to make talos happy.
Assignee | ||
Comment 14•7 years ago
|
||
> These are shutdown crashes where we kill the content process because because it's still starting up when the parent process is shutting down. The shutdown blockers added in bug 1357486 were meant to prevent this, but it looks like the 1s timeout added in bug 1380267 is not quite enough to make talos happy. Interesting. Note that the patch in bug 1386457 was designed to more strictly synchronize startup and shutdown (because of a bug at startup time in 55 RC1: https://github.com/mozilla-services/screenshots/issues/3257). Is it possible to estimate how often this might affect real world users?
Flags: needinfo?(kmaglione+bmo)
Comment 15•7 years ago
|
||
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #14) > Is it possible to estimate how often this might affect real world users? Probably not very often. Regardless, it should be easy enough to fix by just increasing the shutdown blocker timeout.
Flags: needinfo?(kmaglione+bmo)
Comment 16•7 years ago
|
||
kmag, RyanVM did a build of 57 as 56 with the above patch and it's looking fine in Treeherder. Any differences with the shutdown handling code in 57 that would prevent the crashes in comment 8? https://treeherder.mozilla.org/#/jobs?repo=try&revision=203bee2dc76f9f34a3cde40264b1b4b0ebbbbe09
Flags: needinfo?(kmaglione+bmo)
Comment 17•7 years ago
|
||
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #14) > > These are shutdown crashes where we kill the content process because because it's still starting up when the parent process is shutting down. The shutdown blockers added in bug 1357486 were meant to prevent this, but it looks like the 1s timeout added in bug 1380267 is not quite enough to make talos happy. > > Interesting. Note that the patch in bug 1386457 was designed to more > strictly synchronize startup and shutdown (because of a bug at startup time > in 55 RC1: https://github.com/mozilla-services/screenshots/issues/3257). Actually, this patch will completely break the shutdown blocker that's meant to prevent this kind of issue, since we register the shutdown blocker when the shutdown method is called, and you don't call the shutdown method until the startup() method has resolved, which in the failure case is after shutdown has already started.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 18•7 years ago
|
||
> Actually, this patch will completely break the shutdown blocker that's meant to prevent this kind of issue, since we register the shutdown blocker when the shutdown method is called, and you don't call the shutdown method until the startup() method has resolved, which in the failure case is after shutdown has already started. Oh, ok. Let me see if I understand correctly: On one hand, screenshots needs to call `webextension.shutdown()` without waiting for startup to complete. This starts the shutdown blocker timer, which ensures the process is killed before the main Firefox process exits, avoiding a shutdown crash. On the other hand, if we don't synchronize startup and shutdown, it seems like startup and shutdown could race, which might break the feature. For instance, [1] was caused by two racing startup() calls, but I could imagine a startup/shutdown race if shield were flipping the value the other way (from enabled back to disabled), or if the addon wasn't fully started when updates were applied. Are these edge cases possible? What's the right fix here? Would it be enough to synchronize the startup calls, as in the fix for [1], but not synchronize the shutdown calls? [1] https://github.com/mozilla-services/screenshots/issues/3257#issuecomment-319509605
Flags: needinfo?(kmaglione+bmo)
Comment 19•7 years ago
|
||
Please note here are the 32 and 64 bit artifact builds from the failing Beta push: win32 build (win 7 tests run here) - https://queue.taskcluster.net/v1/task/FnA25Eo4TB24ZAv-JCPJ1Q/runs/1/artifacts/public/build/target.zip win64 build (win 10 tests run here) - https://queue.taskcluster.net/v1/task/E2QxMRIMSQyNwao6PiqsNg/runs/0/artifacts/public/build/target.zip
Comment 20•7 years ago
|
||
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #18) > > Actually, this patch will completely break the shutdown blocker that's meant to prevent this kind of issue, since we register the shutdown blocker when the shutdown method is called, and you don't call the shutdown method until the startup() method has resolved, which in the failure case is after shutdown has already started. > > Oh, ok. Let me see if I understand correctly: > > On one hand, screenshots needs to call `webextension.shutdown()` without > waiting for startup to complete. This starts the shutdown blocker timer, > which ensures the process is killed before the main Firefox process exits, > avoiding a shutdown crash. > > On the other hand, if we don't synchronize startup and shutdown, it seems > like startup and shutdown could race, which might break the feature. For > instance, [1] was caused by two racing startup() calls, but I could imagine > a startup/shutdown race if shield were flipping the value the other way > (from enabled back to disabled), or if the addon wasn't fully started when > updates were applied. Are these edge cases possible? No, in Firefox 56+, the Extension.shutdown method always waits for the last startup to complete before beginning shutdown, and vice versa, so this workaround is entirely unnecessary.
Flags: needinfo?(kmaglione+bmo)
Comment 21•7 years ago
|
||
Try pushes for bug 1389786 support this re-landing once Screenshots 10.12.0 is on Beta.
Depends on: 1389786
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a9df4b7dd62f
Updated•7 years ago
|
Target Milestone: mozilla57 → Firefox 57
Updated•7 years ago
|
Product: Cloud Services → Firefox
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jhirsch)
You need to log in
before you can comment on or make changes to this bug.
Description
•