1422407 - Sync shield recipe-client v80 from Github (commit b332740)

Status: RESOLVED FIXED

(Firefox :: Normandy Client, defect)

Description

[Michael Cooper [:mythmon]]

Changes from v76.1 (current in Nightly) to v80:

b332740 - Add add-on test directory to lint, upgrade eslint-plugin-mozilla to 0.4.9 and fix raised errors
473106d - Add high population market to preference experiments.
a4f4fb2 - Fix TC lint error.
02356f8 - Bug 1416003 - Handle preference studies on prefs with only user-branch values
c59f20f - Update recipe-client-addon to use eslint 4 and latest eslint-plugin-mozilla.

808de48 - PR #1100 - Revert study preferences to empty when appropriate.
b2d0f66 - Bug 1402528 - recipe-client-addon: Watch prefs and enable/disable as needed.
4a7bf80 - Cachebust Taskcluster docker image.
0dc3796 - Fixes #1100 Reset pref studies to the correct value when stopping.
1c5fac1 - No need to specify Cc/Ci/Cu as global in head_xpc.js
1235959 - #354 - Use the correct preference for telemetry.

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1422407 - Sync shield recipe-client v80 from Github (commit b332740)

Review commit: https://reviewboard.mozilla.org/r/204718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204718/

Comment 2

[:Gijs (he/him)]

(In reply to Mike Cooper [:mythmon] from comment #0)
> b2d0f66 - Bug 1402528 - recipe-client-addon: Watch prefs and enable/disable
> as needed.

I don't seem to have access to this, can you CC me?

Comment 3

[:Gijs (he/him)]

Comment on attachment 8933776 [details]
Bug 1422407 - Sync shield recipe-client v80 from Github (commit b332740)

https://reviewboard.mozilla.org/r/204718/#review210330

This generally looks OK, though I'm a bit surprised about some of the changes looking new to me (so I lack some context). If you can add some context post-facto that would be helpful. :-)

::: browser/extensions/shield-recipe-client/bootstrap.js:108
(Diff revision 1)
>            default:
>              // This should never happen
>              log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`);
>          }
>        } catch (e) {
> -        if (e.result == Cr.NS_ERROR_UNEXPECTED) {
> +        if (e.result === Cr.NS_ERROR_UNEXPECTED) {

A little surprised by this change, mostly because it's the first thing that greeted me in the diff, because it seems so minor, because both of these are going to be numbers, so it doesn't really matter, but also because in-tree frontend code normally prefers == over ===, so I think this was correct as-is. But I've also gotten into too many arguments over this before that I don't actually think it's worth reverting. Mostly I'm just confused why this is in the diff at all...

::: browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js:384
(Diff revision 1)
> +    // tries to change them. Settings this back to true here allows
> +    // that to happen. Not doing this causes popPrefEnv to hang forever.

Ugh. Thanks for reminding me to fix that bug in popPrefEnv at some point. :-\

Comment 4

[Michael Cooper [:mythmon]]

I've CCed you on bug 1402528.

(In reply to :Gijs from comment #3)
> Comment on attachment 8933776 [details]
> Bug 1422407 - Sync shield recipe-client v80 from Github (commit b332740)
> 
> https://reviewboard.mozilla.org/r/204718/#review210330
> 
> This generally looks OK, though I'm a bit surprised about some of the
> changes looking new to me (so I lack some context). If you can add some
> context post-facto that would be helpful. :-)

I imagine most of the unfamiliar changes came from other reviewers while you were out. If you have any specific questions besides the below, I'd be happy to go through them. Thanks for looking over the patch.

> ::: browser/extensions/shield-recipe-client/bootstrap.js:108
> (Diff revision 1)
> >            default:
> >              // This should never happen
> >              log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`);
> >          }
> >        } catch (e) {
> > -        if (e.result == Cr.NS_ERROR_UNEXPECTED) {
> > +        if (e.result === Cr.NS_ERROR_UNEXPECTED) {
> 
> A little surprised by this change, mostly because it's the first thing that
> greeted me in the diff, because it seems so minor, because both of these are
> going to be numbers, so it doesn't really matter, but also because in-tree
> frontend code normally prefers == over ===, so I think this was correct
> as-is. But I've also gotten into too many arguments over this before that I
> don't actually think it's worth reverting. Mostly I'm just confused why this
> is in the diff at all...

This happened in a PR where Standard8 was working on linting. I assumed a newly introduced eslint rule made this required, and didn't worry too much about it. https://github.com/mozilla/normandy/commit/526e15d9d1973e1e61ff78e17d03fa7c72ca2c6b

> :::
> browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js:
> 384
> (Diff revision 1)
> > +    // tries to change them. Settings this back to true here allows
> > +    // that to happen. Not doing this causes popPrefEnv to hang forever.
> 
> Ugh. Thanks for reminding me to fix that bug in popPrefEnv at some point. :-\

I took a stab at doing this a few weeks ago, and it ended up being over my head. It would be nice to have a fix.

Comment 5

[:Gijs (he/him)]

(In reply to Mike Cooper [:mythmon] from comment #4)
> I've CCed you on bug 1402528.

Thanks, that helps.

> I imagine most of the unfamiliar changes came from other reviewers while you
> were out.

Oh ugh, I am a dummy. Yes, of course.


> > > +    // tries to change them. Settings this back to true here allows
> > > +    // that to happen. Not doing this causes popPrefEnv to hang forever.
> > 
> > Ugh. Thanks for reminding me to fix that bug in popPrefEnv at some point. :-\
> 
> I took a stab at doing this a few weeks ago, and it ended up being over my
> head. It would be nice to have a fix.

I originally filed this as bug 1323779, which has a reasonable amount of context. If there's another bug in that pref env code that you filed, do let me know. I'll see if I can find time to address this in the near future.

Comment 6

[:Gijs (he/him)]

(to be 100% clear, I think this is good to land and don't have any other questions. :-) )

Comment 7

[Michael Cooper [:mythmon]]

(In reply to :Gijs from comment #5) 
> I originally filed this as bug 1323779, which has a reasonable amount of
> context. If there's another bug in that pref env code that you filed, do let
> me know. I'll see if I can find time to address this in the near future.

I didn't file another bug. I pushed some naive "fixes" to Try and saw massive breakage, and didn't look much further.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a726c48b2540
Sync shield recipe-client v80 from Github (commit b332740) r=Gijs

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome  browser/extensions/shield-recipe-client/test/browser/browser_Heartbeat.js r=backout on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a726c48b254057c983e52c254a6fc169dd5e1981

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=149672048&repo=autoland

Backout: https://hg.mozilla.org/integration/autoland/rev/0eea6706d04ad431d12911173b7406a1f23a3b6d

Comment 10

[Michael Cooper [:mythmon]]

I think the error was bug 1338842, an existing intermittent that isn't related to this patch. Can we re-land this?

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Unlikely, look at the many occurrences of that failure in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0cb270dda57624ae29346c6da60292280589f728&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 12

[Michael Cooper [:mythmon]]

Gijs, I'm at a loss here. I don't see anything in this patch that could have changed the intermittent in bug 1338842. I also don't see any way to move bug 1338842 forward, like I mentioned in that bug. This is blocking some important features in Shield. Do you have any insight?

Comment 13

[:Gijs (he/him)]

It looks like you're reverting the changes from https://hg.mozilla.org/mozilla-central/rev/976f45726ed5 ( bug 1416153 ). I assumed this was intentional, but looking at hg log, I'm now less convinced that it is intentional. Certainly on my local Windows machine, if I graft a726c48b2540 onto current central and run:

./mach mochitest --verify browser/extensions/shield-recipe-client//test/browser/browser_Heartbeat.js

I can reproduce the timeouts, and they go away if I revert the changes to that file.

Does that answer the question? :-)

(Sorry for not picking this up in review - I kind of just assumed it was an intentional part of some of the other changes - it certainly looked like a simplification.)

Comment 14

[:Gijs (he/him)]

https://groups.google.com/forum/#!topic/mozilla.dev.platform/naB6gaevSSo and the contents of bug 1416153 may also help understand why the change leads to timeouts.

Out of curiosity, what's your merge strategy for generating these patches?

Either way, I also apologize for forgetting to mention to :arai in bug 1416153 that the test change should have been upstreamed to github. :-(

Comment 15

[Michael Cooper [:mythmon]]

Comment on attachment 8933776 [details]
Bug 1422407 - Sync shield recipe-client v80 from Github (commit b332740)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204718/diff/1-2/

Comment 16

[Michael Cooper [:mythmon]]

That looks like it's probably exactly the problem. I should have caught this sooner, and would of if I actually read my patch :(

I've re-applied the patch, and upstreamed it to our Github repo. It's back up for review.

> Out of curiosity, what's your merge strategy for generating these patches?

The strategy is really naive and manual. First I look over recent history to browser/extensions/shield-recipe-client by hand, to make sure there isn't anything that needs to be upstreamed (which is an error prone process, as evidenced here). After applying all of the things I find there, I run a script we have in the github repo [0]. It deletes everything in browser/extension/shield-recipe-client, performs some light modifications to the files in the github repo, and then copies them over to m-c.

This is not great, and we keep hitting problems like this. I would like a better strategy, but haven't had time to work on one.

To fix the problem here, I read the patch in bug 1415163, and then copied all the changed files from a checkout of m-c that had the patch into the Normandy git repo. Again, very manual and not very reliable, but it works just well enough that it hasn't been a priority to fix.

Comment 17

[:Gijs (he/him)]

(In reply to Mike Cooper [:mythmon] from comment #16)
> This is not great, and we keep hitting problems like this. I would like a
> better strategy, but haven't had time to work on one.

I haven't thought about this for very long, so perhaps I'm missing something, but could you do something like:

git diff -r last-rev-synced -r rev-we-want-to-sync-now > foo.txt

cd mozilla-repo/browser/extensions/shield-recipe-client

patch -p1/2/3 (not sure) path/to/foo.txt

?

That would basically apply the diff from previous-sync version to currently-in-m-c version, and seems like it may be less error-prone? Maybe? But maybe I'm missing something, too.

And, to be fair, the strategy comment 16 described is still also how I do reader mode syncs - but those only involve 2 files so it's easier to manually scan for changes in m-c.

Comment 18

[Michael Cooper [:mythmon]]

That kind of approach might work well. I think especially tracking last-rev-synced in the repo would be useful. I think one issue is that patch can't simply apply some of our changes. For example, install.rdf in the git repo contains @NORMANDY_VERSION@, which we replace with info from our git tags. There are a few things like this.

Maybe the way forward involves removing these extra steps beyond what `patch` can do. Then your approach would work without modifications.

We can probably talk about this more next week, if you'd like.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ad65432822
Sync shield recipe-client v80 from Github (commit b332740) r=Gijs

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a3ad65432822