Closed Bug 1444965 Opened 3 years ago Closed 3 years ago

Fix search composition migration for beta and nightly

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 blocking verified
firefox61 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files, 2 obsolete files)

Bug 1444965 - Fix search composition migration for beta and nightly.
59 bytes, text/x-review-board-request
mak
: review+
mythmon
: review+
Details
Beta 60 patch
24.86 KB, patch
RyanVM
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Bug 1426216 added a UI migration step for showing search suggestions ahead of bookmarks/history for users with unified url/search bars.  Bug 1444548 backed it out on release because it affected existing users, which wasn't the intention.  It's still on 60 (beta) and 61 (nightly).  We need to fix it on 60 and 61 so that it works as intended.
Flags: qe-verify+
[Tracking Requested - why for this release]:
We will need to either back out the search composition migration on 60, like we did on 59 (at the last minute), or we will need to get the fix for it into 60, once we have a fix, which we don't right now.
tracking-firefox60: --- → ?
Marco, do you have any ideas for how we can/should determine whether a user is a "new" install and is therefore eligible to have search suggestions first?

At first I was thinking we should simply sniff for the Shield study.  Only new installs on 57+ get the study, and they are the users we want to migrate.  But of course that's no good because eventually the study will stop running.

I'm not sure how to do it except to use ProfileAge.jsm and compare to the 57 time frame.  I looked at the study recipe [1], and that seems to be how they are choosing users:

> normandy.channel == 'release' &&
> normandy.version >= '57.0' &&
> ((!normandy.telemetry.main.environment.profile.creationDate && normandy.isFirstRun) ||
>  normandy.telemetry.main.environment.profile.creationDate > 17483) &&
> 'browser.search.widget.inNavBar'

17483 is the number of days since epoch = new Date(1510531200000) => Date 2017-11-13T00:00:00.000Z.  57 was released on 2017-11-14.

[1] https://normandy.cdn.mozilla.net/api/v1/recipe/?enabled=1
Flags: needinfo?(mak77)
(In reply to Drew Willcoxon :adw from comment #2)
> Marco, do you have any ideas for how we can/should determine whether a user
> is a "new" install and is therefore eligible to have search suggestions
> first?

I was just thinking about that today, I don't think we have a way to tell "this profile has been created in version X".
It may also be complicate to have something like that because potentially users can move files across profiles.

> I'm not sure how to do it except to use ProfileAge.jsm and compare to the 57
> time frame.

Yes, I ended up thinking the same, check browser version >= 57 and profile created after the 57 release.
Just ensure with Javaun 57 is what we want. This change was expected to hit new profiles created in 59, not 57. On the other side, 57 is when we released the unified urlbar and Quantum.
Flags: needinfo?(mak77)
Also, pay attention to ESR, it will be 60 and likely its profiles will satisfy the condition. What do we want for ESR?
I've pinged Javaun.  I'll request review once I hear back and once I manually test this.
Talked with Javaun Over IRC:

(In reply to Marco Bonardo [::mak] from comment #3)
> Just ensure with Javaun 57 is what we want.

Yes

(In reply to Marco Bonardo [::mak] from comment #4)
> Also, pay attention to ESR, it will be 60 and likely its profiles will
> satisfy the condition. What do we want for ESR?

My understanding is that he'd first like to know/decide whether users coming from ESR 52 get unified or not.
Javaun, another question:  If I create a profile in a 57+ release, then I get the Shield study, my bars are unified, and I get search suggestions first.  If I then disable Shield (which I presume simulates the study's uninstallation), I revert to history first.  If I then upgrade to 60, what should happen?  With my patch here*, I suddenly get search suggestions first again because my bars are unified.  (IMO I should stay with whatever I had before the upgrade.  Not sure how to actually implement that though.)

* Actually this patch is broken, I've got a new one locally.
Flags: needinfo?(jmoradi)
(In reply to Drew Willcoxon :adw from comment #8)
> (IMO I should stay with whatever I had before the upgrade.  Not sure how to
> actually implement that though.)

Maybe sniffing for the study is the right thing to do after all!  If it's installed, then continue whatever behavior the user currently sees with it.  If it's not installed, then don't try to do anything at all.
Drew, is there an ETA for this work?  Thanks.
I need a response from Javaun to my last two comments.  Once there's clarity around the expected behavior, implementing it should be straightforward.

If that's doable and not too much LOE, we can make it work (to sniff for Shield). The users abandoning Shield could be high (because of technical drops, not user opt-out which is harder). I think we could live with that edge case though, especially with the new UI pref.


NI Matt Grimes, because one big question is will the Shield be there when the migration sniffs. The Shield study will be set to expire on upgrade, the migration should run first, but I would rather be in a situation where we hard reset a bunch of users who've dropped Shield than have a condition where we sniff the shield and that fails.

Drew what's the complexity of the sniff? Matt, when does the shield end in the startup sequence?
Flags: needinfo?(jmoradi) → needinfo?(mgrimes)
I'd say you should ignore the branches of the study completely and target based on version, profile creation date AND the unification pref. We intentionally kicked users out of the study if they switched back to two bars. We also gave some users the history first experience in the study (control). Given what we've learned from the experiment we think that a search first experience is better. If we continue with the current behavior those users that were in the control group will keep a sub-optimal experience. They might be used to it at this point, so that's probably a call for Javaun. +mythmon to comment on the Shield startup sequence.
Flags: needinfo?(mgrimes) → needinfo?(mcooper)
I think sniffing for the study will work fine, but I'd recommend not trying to read the prefs to infer what Shield did. Instead you  can read Shield's data stores about preferences directly, which seems a lot more reliable to me. The code to do that would look something like this:

    // this path needs adjusted to resource://shield- recipe-client on pre-v60
    ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm");
    try {
      const experiment = await PreferenceExperiments.get('experiment-name');

      if (experiment.expired) {
        // user was in the study, but left on or after `experiment.lastSeen`.
      } else {
        // user is currently in the study
      }
    } catch (e) {
      // user has never been in the study
    }

If you wanted to be super clever, you could then also end the Shield Study like below. This isn't needed, but would be convenient. It would let us track when people left because of this automatic migration vs some other method. The "reason" key is what we would use to do that tracking.

    await PreferenceExperiments.stop('experiment-name', {
      resetValue: false,
      reason: "external:search-ui-migration", // use whatever you would like
    })

If you don't want to go with this approach, it should be safe to use the plan you had. For any existing profiles, Normandy won't unenroll right away, probably delayed at least a minute. It is possible that the default pref will be in an inconsistent state though, if you run too early. If you want to be very sure, there is an event you could listen for to know when Normandy has definitely set all the prefs it's going to during startup:

    Services.obs.notifyObservers(null, "shield-init-complete");

If you run right after this event is fired, there shouldn't be any race conditions.
Flags: needinfo?(mcooper)
(In reply to Matt Grimes [:Matt_G] from comment #13)
> I'd say you should ignore the branches of the study completely and target
> based on version, profile creation date AND the unification pref.

That means that some users will see the behavior in comment 8, right?  Which seems jarring to me.  Or will comment 8 not happen in practice?  I'm assuming that once we stop/uninstall the study, users on 57-59 will revert to the default/standard behavior of getting history first.  Is that not the case?
Flags: needinfo?(mgrimes)
(In reply to Drew Willcoxon :adw from comment #15)
> (In reply to Matt Grimes [:Matt_G] from comment #13)
> > I'd say you should ignore the branches of the study completely and target
> > based on version, profile creation date AND the unification pref.
> 
> That means that some users will see the behavior in comment 8, right?  Which
> seems jarring to me.  Or will comment 8 not happen in practice?  I'm
> assuming that once we stop/uninstall the study, users on 57-59 will revert
> to the default/standard behavior of getting history first.  Is that not the
> case?

If they changed the pref manually (or opted out) they would have no study branch as they'd no longer be enrolled. My point was that strictly adhering to the branch assignment is probably not be optimal either since branch assignment was done at random.  When we end study, users will return to whatever the default is. The default was history first when we launched but if the new default is search first, they'll get search first. If at some point the user was in the treatment group (search first) and manually changed to history first they would have been kicked out of the study, so ending it would have no impact on them at this point. Mythmon can confirm my logic in case I've missed something. There's probably not a "perfect" solution here for all edge cases, so I trust Javaun's judgement here.
Flags: needinfo?(mgrimes)
Users who are stuck on 57-59 when the Shield study ends (because they don't bother to upgrade before we end it, for example) and then upgrade to 60 -- that's potentially a large number of users, isn't it?  It doesn't seem like an edge case to me necessarily.

My concern is that we should try to provide continuity to users before and after upgrading to 60.  If you're seeing history first before upgrading, you should see it first after.  If you're seeing search suggestions first before, you should see them first after.  That seems doable technically.  Do we want to do that from a product POV?  I'm still not clear on what Javaun thinks about that.
Thanks Drew, I'm all for continuity. My two concerns are 1. Level of effort to create/test, and also 2. if we do a "sniff" whether it's brittle and it could cause the migration to fail. It sounds like Mythmon is saying that Shield will at least reliably run *after* your migration (right?) so we're not conditioned on something that may not be there.

I'll be in the weekly tomorrow and can discuss.
Attached patch WIP using PreferenceExperiments (obsolete) — — Splinter Review 
Mike, thanks very much for your code snippets, very helpful.  Could you please check how I'm using PreferenceExperiments in this WIP?  The main thing I'm wondering about is the lastSeen comparison I'm doing -- I'm concerned about this code racing the experiment uninstallation.  Would that even be a problem?  This code runs on startup, and I presume that could be when the experiment is possibly uninstalled.
Attachment #8962512 - Flags: feedback?(mcooper)
For completeness, here's an updated patch.  The previous one was missing a line.  This one actually works -- not that you need to run/build this, Mike.  I'd just like your general feedback on my use of PreferenceExperiments and anything I might be missing.

I did do some brief testing with this, and it seems to work.  The experiment is nicely uninstalled on upgrading from a profile created with 58, and the profile keeps suggestions first.  And on upgrading from 56 to 58 to a build with this, the profile keeps history/bookmarks first.
Attachment #8962512 - Attachment is obsolete: true
Attachment #8962512 - Flags: feedback?(mcooper)
Attachment #8962547 - Flags: feedback?(mcooper)
Comment on attachment 8962547 [details] [diff] [review]
WIP 2 using PreferenceExperiments

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

::: browser/components/nsBrowserGlue.js
@@ +2295,5 @@
> +
> +    // If the experiment is expired but was last seen "recently" (within 5
> +    // minutes), then assume that this migration is racing its uninstallation,
> +    // and the user has been seeing the experiment UX.  Therefore, use the
> +    // experiment's pref value, not the current pref value.

This isn't quite correct. Normandy triggers an uninstallation when the recipe was not seen. That means that the time stamp will never be less than 24 hours old. This isn't strictly wrong, but I also expect it will never trigger.
(In reply to Mike Cooper [:mythmon] from comment #21)
> This isn't quite correct. Normandy triggers an uninstallation when the
> recipe was not seen. That means that the time stamp will never be less than
> 24 hours old. This isn't strictly wrong, but I also expect it will never
> trigger.

Just to follow up, I chatted with Mike, and if the study is uninstalled, it will happen minutes after final-ui-startup, which is when UI migration happens.  So this won't be a problem, so I removed that part of the patch.
Attachment #8962547 - Attachment is obsolete: true
Attachment #8962547 - Flags: feedback?(mcooper)
Comment on attachment 8958584 [details]
Bug 1444965 - Fix search composition migration for beta and nightly.

https://reviewboard.mozilla.org/r/227490/#review237500

From a code point of view it looks ok, but honestly this thing is still underspecified for me to be able to review it, see my question below.

Also note, we must land soon, because if someone else adds a v66 UI migration before us, beta and nightly will start disagreeing on the version and we may be in trouble.

::: browser/components/nsBrowserGlue.js:2280
(Diff revision 3)
> -                                   "general:5,suggestion:Infinity");
> -      }
> +    // for a Shield study that changed the browser.urlbar.matchBuckets pref in
> +    // order to show search suggestions above history/bookmarks in the urlbar
> +    // popup.  This uninstalls that study.  (It's actually slightly more
> +    // complex.  The study set the pref to several possible values, but the
> +    // overwhelming number of profiles in the study got search suggestions
> +    // first, followed by history/bookmarks.)

Based on the code we are putting history/bookmarks first for any user that was in the experiment but was not in the "suggestions first" branch. That is at least the control group I assume, provided we didn't have many more branches?

That is pretty much comment 13: "Users that were in the control group will have a sub-optimal experience". Is this what was decided in the end? I can't find a final comment about this.

I'm honestly scared of reviewing this without a tabular view of all the branches and what we should expect, that would also be useful for QA.

I made my own flow-chart to understand more of this, and I'm still not sure what to expect for the users that are in the experiment but don't have "suggestion:4,general:5", nor how many of those there are.

::: browser/components/nsBrowserGlue.js:2302
(Diff revision 3)
> +      let topic = "shield-init-complete";
> +      Services.obs.addObserver(function obs() {
> +        Services.obs.removeObserver(obs, topic);
> +        resolve();
> +      }, topic);
> +    });

Off-hand it looks like you could just have a this._shieldInitComplete PromiseUtils.deferred that is resolved when shield-init-complete is invoked, and await its promise.
(In reply to Marco Bonardo [::mak] from comment #26)
> Comment on attachment 8958584 [details]
>
> I'm honestly scared of reviewing this without a tabular view of all the
> branches and what we should expect, that would also be useful for QA.
> 
> I made my own flow-chart to understand more of this, and I'm still not sure
> what to expect for the users that are in the experiment but don't have
> "suggestion:4,general:5", nor how many of those there are.

To help alleviate some of these concerns, here is the definition of the experiment, which includes the branches: https://normandy.cdn.mozilla.net/api/v1/recipe/346/

The relevant bit from there is

  "branches": [
    {
      "ratio": 1,
      "slug": "gen3sug6",
      "value": "general:3,suggestion:6"
    },
    {
      "ratio": 97,
      "slug": "sug4gen5",
      "value": "suggestion:4,general:5"
    },
    {
      "ratio": 1,
      "slug": "gen1sug4gen4",
      "value": "general:1,suggestion:4,general:4"
    },
    {
      "ratio": 1,
      "slug": "sug1gen4sug4",
      "value": "suggestion:1,general:4,suggestion:4"
    }
  ]

For example, to answer your question about how many users are in the experiment but don't have "suggestion:4,general:5": we see that that branch has a ratio of 97, and the sum of all ratios is 100, so 97% of users in the experiment have "suggestion:4,general:5".
Comment on attachment 8958584 [details]
Bug 1444965 - Fix search composition migration for beta and nightly.

https://reviewboard.mozilla.org/r/227490/#review237592

OK, it sounds fair.
Attachment #8958584 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #26)
> From a code point of view it looks ok, but honestly this thing is still
> underspecified for me to be able to review it, see my question below.

Yeah, I understand.

In summary, we want to preserve whatever the user is seeing when they upgrade to 60.  Regardless of why they're seeing it.  At least IMO, and I think Javaun has agreed.

> ::: browser/components/nsBrowserGlue.js:2280
> (Diff revision 3)
> > -                                   "general:5,suggestion:Infinity");
> > -      }
> > +    // for a Shield study that changed the browser.urlbar.matchBuckets pref in
> > +    // order to show search suggestions above history/bookmarks in the urlbar
> > +    // popup.  This uninstalls that study.  (It's actually slightly more
> > +    // complex.  The study set the pref to several possible values, but the
> > +    // overwhelming number of profiles in the study got search suggestions
> > +    // first, followed by history/bookmarks.)
> 
> Based on the code we are putting history/bookmarks first for any user that
> was in the experiment but was not in the "suggestions first" branch.

No, I don't think that's true.  This patch should be able to handle any and every branch that's in the experiment.  Each branch has a particular pref value (see Mike's comment).  The patch preserves that value and doesn't care about which branch it comes from.

Mike, please correct me if I'm wrong?
I should add a test task that installs the experiment with one of the 1-ratio pref values, and make sure that value is preserved.  (It should be, I expect such a test to pass.)
Added the test task mentioned in my previous comment.

(In reply to Marco Bonardo [::mak] from comment #26)
> ::: browser/components/nsBrowserGlue.js:2302
> (Diff revision 3)
> > +      let topic = "shield-init-complete";
> > +      Services.obs.addObserver(function obs() {
> > +        Services.obs.removeObserver(obs, topic);
> > +        resolve();
> > +      }, topic);
> > +    });
> 
> Off-hand it looks like you could just have a this._shieldInitComplete
> PromiseUtils.deferred that is resolved when shield-init-complete is invoked,
> and await its promise.

That would require modifying nsBrowserGlue's _init, creating the deferred and setting it to _shieldInitComplete.  Not a problem, but I like how the current patch doesn't add any extra work to the non-migration path other than setting a bool.  So I left this as is.  If we add other consumers that need to wait until Shield inits, we can revisit.
(In reply to Drew Willcoxon :adw from comment #29)
> > Based on the code we are putting history/bookmarks first for any user that
> > was in the experiment but was not in the "suggestions first" branch.
> 
> No, I don't think that's true.  This patch should be able to handle any and
> every branch that's in the experiment.  Each branch has a particular pref
> value (see Mike's comment).  The patch preserves that value and doesn't care
> about which branch it comes from.

I see, it's this code:
prefValue = prefValue || "general:5,suggestion:Infinity";
Thanks.
Yes, exactly
Comment on attachment 8958584 [details]
Bug 1444965 - Fix search composition migration for beta and nightly.

https://reviewboard.mozilla.org/r/227490/#review237732

I agree with Marco about using a deferred for `_shieldInitComplete`, but otherwise this looks good to me.
Attachment #8958584 - Flags: review?(mcooper) → review+
Thanks Mike.  That's 2 to 1 for the _shieldInitComplete deferred change, so I'll make that change and then land this.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60bd2e867d35
Fix search composition migration for beta and nightly. r=mak,mythmon
browser/base/content/test/performance/browser_startup.js failed because now we're loading PromiseUtils.jsm (for the _shieldInitComplete deferred).  I don't want to mess with that test, so I'm just going to go back to using a bool and a promise inside the migration method instead of using a deferred.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8be24c44ac79
Fix search composition migration for beta and nightly. r=mak,mythmon
Flags: needinfo?(adw)
https://hg.mozilla.org/mozilla-central/rev/8be24c44ac79
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 60 → Firefox 61
Please request Beta approval on this when you get a chance.
Flags: needinfo?(adw)
Attached patch Beta 60 patch — — Splinter Review 
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1426216
[User impact if declined]: Users with profiles older than 57 and unified search bars will see search suggestions first, which is not what we want
[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]: Yes, we've discussed the steps over email and docs
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Moderate
[Why is the change risky/not risky?]: We botched the previous attempt at this and had to back it out on RC last minute, but we think this one is good
[String changes made/needed]: None
Flags: needinfo?(adw)
Attachment #8964357 - Flags: approval-mozilla-beta?
Comment on attachment 8964357 [details] [diff] [review]
Beta 60 patch

Let's get this baking ASAP. Approved for 60.0b9.
Attachment #8964357 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue is verified fixed using Ubuntu 16.04 64bit, macOS 10.13 and Windows 10 64bit.

We used profiles created on Fx 56 and Fx 57 (for both Beta and Nightly). The bookmarks/history are displayed first and this behavior is preserved after updating to 60.0b11 and 61.0a1 builds.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.