Closed Bug 1397415 Opened 3 years ago Closed 3 years ago

Sync shield-recipe-client v73 from GitHub (commit e96eea7)

Categories

(Firefox :: Normandy Client, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: osmose, Assigned: osmose)

References

Details

Attachments

(2 files)

There have been 4 PRs since the last sync (bug 1388823) that affected the add-on:

- #1002: Fix bug 1393257: Stop in-progress studies when opt-out pref changes.
  https://github.com/mozilla/normandy/pull/1002 

- #1010: Bug 1392738: Update how we open the new preferences UI.
  https://github.com/mozilla/normandy/pull/1010

- #1024: Bug 1371350: Delay almost all startup tasks until after sessionstore-windows-restored
  https://github.com/mozilla/normandy/pull/1024

- #1029: Fix #856: Add type parameter to preference experiment annotations.
  https://github.com/mozilla/normandy/pull/1029

We'd also like to uplift these to beta, but bug 1349689, which landed in mozilla-central, also modified Shield with changes that we don't want to uplift. This mean the uplift patch will be different from the sync one; I'll rebase and apply the patches to mozilla-beta manually and attach a second patch to use for uplift separate from the mozilla-central sync.
Comment on attachment 8905216 [details]
Bug 1397415: Sync shield-recipe-client v73 from GitHub (commit e96eea7)

https://reviewboard.mozilla.org/r/177000/#review182034

I mean, rs=me because I think this basically already got reviewed - though I'm a little confused about the vendor changes, especially propTypes where the before and after look basically the same to me, so I'm not sure what changed there... Haven't checked the rest really, let me know if I should (I assume there were no conflicts when merging this? :-)
Attachment #8905216 - Flags: review?(gijskruitbosch+bugs) → review+
This patch includes the following PRs:

- #1002: Fix bug 1393257: Stop in-progress studies when opt-out pref changes.
  https://github.com/mozilla/normandy/pull/1002

- #1024: Bug 1371350: Delay almost all startup tasks until after sessionstore-windows-restored
  https://github.com/mozilla/normandy/pull/1024

- #1029: Fix #856: Add type parameter to preference experiment annotations.
  https://github.com/mozilla/normandy/pull/1029

It also includes the fix from bug 1392648, which one of the PRs relies upon.
Attachment #8905269 - Flags: review?(gijskruitbosch+bugs)
(In reply to Michael Kelly [:mkelly,:Osmose] from comment #4)
> Created attachment 8905269 [details] [diff] [review]
> Sync shield-recipe-client v73 from GitHub (commit e96eea7) to beta

This is the patch we should use for uplifting this to Beta. It is missing the PR for bug 1392738 (which relied on the changes in mozilla-central when the old preferences organization was removed) and includes the patch for bug 1392648. Otherwise it should be the same as attachment 8905216 [details].
Pushed by mkelly@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ef48b1ea11b
Sync shield-recipe-client v73 from GitHub (commit e96eea7) r=Gijs
https://hg.mozilla.org/mozilla-central/rev/4ef48b1ea11b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8905269 [details] [diff] [review]
Sync shield-recipe-client v73 from GitHub (commit e96eea7) to beta

Review of attachment 8905269 [details] [diff] [review]:
-----------------------------------------------------------------

Again, rs=me because this all got reviewed already.
Attachment #8905269 - Flags: review?(gijskruitbosch+bugs) → review+
we see some reduced fileIO with this:
== Change summary for alert #9262 (as of September 06 2017 21:53 UTC) ==

Improvements:

  7%  tp5n nonmain_startup_fileio windows7-32 pgo e10s     1,695,344.50 -> 1,568,891.92
  7%  tp5n nonmain_startup_fileio windows7-32 opt e10s     1,706,547.15 -> 1,594,771.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9262
Comment on attachment 8905269 [details] [diff] [review]
Sync shield-recipe-client v73 from GitHub (commit e96eea7) to beta

Approval Request Comment
[Feature/Bug causing the regression]:
This is a combination of fixes for four bugs (since Normandy does development in Github and syncs to m-c):

- Bug 1393257 - End in-progress add-on studies when the opt-out preference is switched off
- Bug 1371350 - shield shouldn't load anything before opening and painting the first browser window
- Bug 1392648 - SHIELD browser-chrome tests are going to permaleak when Gecko 57 merges to Beta on 2017-09-20
- #856 - Annotate studies with new experiment type option (Github: https://github.com/mozilla/normandy/issues/856

[User impact if declined]:
- Users will not be unenrolled from in-progress feature experiments if they choose to opt-out of experiments globally.
- Firefox startup will be slower due to increased fileio from the performance regression.
- Telemetry infrastructure for doing automated analysis of feature experiments will continue to be at risk of falling over if we send experiments to large populations.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes. The performance fix and leak have been verified fixed in automation, and QA has verified the opt-out pref fix as part of their larger test plan for opt-out studies.

[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?]:
Shouldn't be.

[Why is the change risky/not risky?]:
All the changes are mostly to non-user-visible functionality, and are easily rolled back with a system add-on update if we find something wrong.

[String changes made/needed]:
None
Attachment #8905269 - Flags: approval-mozilla-beta?
Comment on attachment 8905269 [details] [diff] [review]
Sync shield-recipe-client v73 from GitHub (commit e96eea7) to beta

Fix for some perf and leaking issues in Shield.  Let's uplift this for beta 10.
Attachment #8905269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1399245
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.