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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: osmose, Assigned: osmose)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
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+
Assignee

Comment 4

2 years ago
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)
Assignee

Comment 5

2 years ago
(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].

Comment 6

2 years ago
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: 2 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
Assignee

Comment 10

2 years ago
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+
Assignee

Updated

2 years ago
Blocks: 1399245

Updated

Last year
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.