Closed Bug 1388823 Opened 2 years ago Closed 2 years ago

Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

Categories

(Firefox :: Normandy Client, defect)

defect
Not set

Tracking

()

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

People

(Reporter: mythmon, Assigned: mythmon)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The major change to the add-on since the last sync (Bug 1384652, v61) is opt-out studies support.
Summary: Sync shield-recipe-client v64 from GitHub (commit c3bfdcb9) → Sync shield-recipe-client v65 from GitHub (commit ff1b680c)
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Comment on attachment 8895921 [details]
Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

https://reviewboard.mozilla.org/r/167202/#review172786

There are some errors on try to do with the all_files_referenced.js checks. I think it (for whatever reason) isn't realizing you *are* referencing the content / frame scripts. I don't know why. I suggest you add them to the whitelist to get unblocked for now. There's also an unused whitelist entry warning for IndexedDB.jsm that I think you can then just remove, maybe?

I thought we removed the dependency on ajv again? I don't see it being used apart from using Cu.unload.

I'm also confused because this PR seems to include about:studies support but that isn't mentioned either here or in bug 1388823 . Is that just part of opt-out studies support?

Finally, your trypush only ran mochitest-e10s-browser-1, not the other however many. Was that intentional? It rather seems like the others should be running too...
Attachment #8895921 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #2)
> Comment on attachment 8895921 [details]
> Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)
> 
> https://reviewboard.mozilla.org/r/167202/#review172786
> 
> There are some errors on try to do with the all_files_referenced.js checks.
> I think it (for whatever reason) isn't realizing you *are* referencing the
> content / frame scripts. I don't know why. I suggest you add them to the
> whitelist to get unblocked for now. There's also an unused whitelist entry
> warning for IndexedDB.jsm that I think you can then just remove, maybe?

Thanks for debugging this. I'll adjust the exceptions.

> I thought we removed the dependency on ajv again? I don't see it being used
> apart from using Cu.unload.

Nice catch. I think this may have gotten included because I forgot to cleanup a generated files directory. I'll make sure it stays gone.
 
> I'm also confused because this PR seems to include about:studies support but
> that isn't mentioned either here or in bug 1388823 . Is that just part of
> opt-out studies support?

Yes, about:studies is a part of opt-out studies. It is the UI that allows users to opt out of out-out studies.

>> Finally, your trypush only ran mochitest-e10s-browser-1, not the other
> however many. Was that intentional? It rather seems like the others should
> be running too...

Oops. I copied another try line without reading it too closely. I'll make sure to run a better set of tests this time.
Comment on attachment 8895921 [details]
Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

https://reviewboard.mozilla.org/r/167202/#review172944

r=me assuming try comes back green
Attachment #8895921 - Flags: review?(gijskruitbosch+bugs) → review+
Try did not come back green, there were a couple Windows-specific test failures. Those are fixed now. Can you re-r? this?
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87a67b857362
Sync shield-recipe-client v65 from GitHub (commit ff1b680c) r=Gijs
Comment on attachment 8895921 [details]
Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

Approval Request Comment
[Feature/Bug causing the regression]: Shield Opt-out studies
[User impact if declined]: Tab Center studies will not be able to run.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Manual testing is being performed, bugs will be filed blocking bug 1388092. Test plan is at https://wiki.mozilla.org/QA/Shield_Opt_Out_Studies . I don't know if this needs to block the uplift.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Unlikely
[Why is the change risky/not risky?]: This adds new capabilities to Shield, that are not used except for a small percentage of users in experimental groups. Of course, there may be bugs.
[String changes made/needed]: None
Attachment #8895921 - Flags: approval-mozilla-beta?
Depends on: 1390325
Tracking to make sure we follow up on landing and uplifting.
Gijs: I've put up a potential fix for the test errors; the fix is in AboutPages.jsm in the sendStudyList function (I couldn't figure out how to link to the lines but you can see the diff on Github: https://github.com/mozilla/normandy/compare/master...Osmose:child-process-fail). It's otherwise identical to the backed-out patch.

My thinking is that the test failures close about:studies after it fires off a request for studies to the parent process, but before the parent process sends the results back, and it's the attempt to send a message to a non-existent target that is failing during the tests.

I did a try run to test the fix and bc1 and bc5 were fixed (https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddefa1dca3632540412027c2ef340a407772edd7), although other chunks failed due to my test missing fixes from this patch. I've triggered a try run for the updated patch on the test suites in question to see if it works.
Flags: needinfo?(mcooper)
Comment on attachment 8898034 [details]
Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

https://reviewboard.mozilla.org/r/169338/#review174692

rs=me on the changes in https://github.com/mozilla/normandy/compare/master...Osmose:child-process-fail . I guess this is good to go if the tests are green, we can fix the IO stuff in a followup if necessary.
Attachment #8898034 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mkelly@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e76b1b892fc8
Sync shield-recipe-client v65 from GitHub (commit ff1b680c) r=Gijs
Depends on: 1391684
Comment on attachment 8895921 [details]
Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c)

Looks like we have worked out most of the issues, with one known perf test failure that may be from the test methodology. 

Let's try this in 56 beta 5, so we can run studies in beta 56.
Attachment #8895921 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1392648
Depends on: 1392738
Attachment #8895921 - Attachment is obsolete: true
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.