1368477 - Disabled search suggestions are being enabled after update

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170529030204

[Affected versions]:
- Nightly 55.0a1
 
[Affected platforms]:
- All
 
[Steps to reproduce]:
1. Install an older version of Nightly 55 where search suggestions are not enabled by default (http://archive.mozilla.org/pub/firefox/nightly/2017/05/2017-05-20-03-02-04-mozilla-central/)
2. Launch the older version of Nightly 55
3. Focus the URL Bar and start typing
4. In the search suggestions, opt-in notification bar select NO
5. Update to the latest Nightly 55
6. Focus the URL bar and start typing
7. Navigate to about:preferences#general and check the status of the option "Show search suggestions in location bar results"

[Expected result]:
- In step 6 - the search suggestions should be disabled
- In step 7 - the "Show search suggestions in location bar results" box should be unchecked
 
[Actual result]:
- In step 6 - the search suggestions are enabled
- In step 7 - the "Show search suggestions in location bar results" box is checked

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Simona B [:simonab ] from comment #0)
> 1. Install an older version of Nightly 55 where search suggestions are not
> enabled by default
> (http://archive.mozilla.org/pub/firefox/nightly/2017/05/2017-05-20-03-02-04-
> mozilla-central/)
> 2. Launch the older version of Nightly 55
> 3. Focus the URL Bar and start typing
> 4. In the search suggestions, opt-in notification bar select NO
> 5. Update to the latest Nightly 55

Could you please tell me the value of the browser.urlbar.searchSuggestionsChoice pref?
Any version after the 13th of May should also store that pref when you change suggest.searches and we use that pref to set the new value.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Also, did you by chance reuse a profile that was already used on a recent Nightly when installing the older version?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

To better explain comment 2, we don't support downgrade/upgrade paths here, so if you reuse a profile of today's nightly with the old nightly and then upgrade again to today's nightly, it won't work. Ensure you are using a new profile in the old nightly and doing an upgrade from that.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

adding this to tracking, just in case.

Comment 5

[Simona Badau, Desktop QA]

(In reply to Marco Bonardo [::mak] from comment #1)
> Could you please tell me the value of the
> browser.urlbar.searchSuggestionsChoice pref?
> Any version after the 13th of May should also store that pref when you
> change suggest.searches and we use that pref to set the new value.

Strangely, the preference browser.urlbar.searchSuggestionsChoice does not exist if I choose NO in the search suggestions notification bar (looked for it after selecting NO before the update and checked again after the update).
Please note that if I choose Yes, the value of the preference browser.urlbar.searchSuggestionsChoice is set to true.

I used a new profile that was created using the older Nightly version (from May 20).

Comment 6

[Simona Badau, Desktop QA]

Attachment: EnabledSuggestions.mp4

Marco, please see the screencast for more details.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think I know what's the problem, the pref observer in urlbarbindings is activated when "suggest.searches" changes, but if you pick "no" and the default is false, the pref doesn't change. This problem is mitigated until the 23rd build by code that we ran on every startup that mirrored suggest.searches. But now that code is gone.

I suspect that, if you restart the browser once after setting the pref but before upgrading, it will work.
Most users likely will have a session restart between bug 1364002 and Firefox 55, so the problem is mitigated. This could be a workaround to test the feature using previous builds.

It is worth fixing and uplifting this, to really be on the safe side regarding 54 => 55 upgrades. We can also add further mitigation code in Nightly to detect this case.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, basically, you can try this workaround:

> 1. Install an older version of Nightly 55 where search suggestions are not
> enabled by default
> (http://archive.mozilla.org/pub/firefox/nightly/2017/05/2017-05-20-03-02-04-
> mozilla-central/)
> 2. Launch the older version of Nightly 55
> 3. Focus the URL Bar and start typing
> 4. In the search suggestions, opt-in notification bar select NO

====> Restart the browser

> 5. Update to the latest Nightly 55
> 6. Focus the URL bar and start typing
> 7. Navigate to about:preferences#general and check the status of the option
> "Show search suggestions in location bar results"

And in the meanwhile, I'll try to have this fixed asap.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 1368477 - Disable search suggestions if in doubt regarding user choice from the previous version.

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

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8872892 [details]
Bug 1368477 - Disable search suggestions if in doubt regarding user choice from the previous version.

https://reviewboard.mozilla.org/r/144420/#review148236

Makes sense, thanks!

::: browser/components/nsBrowserGlue.js:2030
(Diff revision 1)
> +          // something went wrong in the upgrade path. For example picking "no"
> +          // at the opt-in bar and upgrading immediately wouldn't mirror the pref.

This is only for any profiles where this happened without this patch, right? Since from now on userMadeSearchSuggestionChoice will trigger setting suggest.searches. If that is the case, can you clarify further in the comment?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #10)
> ::: browser/components/nsBrowserGlue.js:2030
> (Diff revision 1)
> > +          // something went wrong in the upgrade path. For example picking "no"
> > +          // at the opt-in bar and upgrading immediately wouldn't mirror the pref.
> 
> This is only for any profiles where this happened without this patch, right?
> Since from now on userMadeSearchSuggestionChoice will trigger setting
> suggest.searches. If that is the case, can you clarify further in the
> comment?

Yes, but it doesn't really matter because versions having the userMadeSearchSuggestionChoice will have already passed this migrateUI step and are opt-out... It would make more sense if we'd plan to uplift it, but I don't see a possibility for that.
Do you still want me to expand the comment?

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

I don't really mind going either way with the comment, this is why I didn't mark it as an issue. But I agree that this isn't a big problem and that is why I suggested to clarify the comment. In my first reading of it I was like "oh, so there is a bug we still need to fix here".

There might be a small chance to get this in 54, b13 (last one before RC) is built tomorrow and if we get an approval today it could happen.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8872892 [details]
Bug 1368477 - Disable search suggestions if in doubt regarding user choice from the previous version.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d29a7708e357
Disable search suggestions if in doubt regarding user choice from the previous version. r=past

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'll try to make a dumbed down one-liner fix for beta. needinfo-ing gchang as an heads-up, let's see what we can do.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Beta one-liner fix

Approval Request Comment
[Feature/Bug causing the regression]: No regression
[User impact if declined]: If the user disabled search suggestions from the opt-in notification and upgrades to 55 in the same session, we may be unable to know whether he picked YES or NO in the notification.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: it will be verified by QA, since QA found the original issue
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a oneliner mirroring a pref value when another pref changes
[String changes made/needed]: none

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8872942 [details] [diff] [review]
Beta one-liner fix

aiui having this in 54 prevents an issue with search suggestion pref when upgrading to 55, so let's take this in beta13.  Replying for Gerry who's on PTO this week.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8872942 [details] [diff] [review]
Beta one-liner fix

NOTE FOR ESR52: This should land on top of bug 1364002.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is related to bug 1364002, it's handling an edge case.
User impact if declined: In some case we may not mirror the pref, and on update we'd not know which choice the user made, we'd then be forced to disable suggestions for those users instead of respecting their choice.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): very low, it's a oneliner setting a pref
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb133d8ca1f1

Comment 21

[Saheda Reza [:Antora]]

I have reproduced this bug on Firefox nightly according to (2017-05-29)

Fixing bug is verified on Latest Nightly ---Build ID:( 20170604030205), User Agent:Mozilla/5.0 (Windows NT 6.1; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows7 32bit

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8872942 [details] [diff] [review]
Beta one-liner fix

This patch is needed for bug 1364002. ESR52+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/079dd3dc77bb

Comment 24

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170605030204

Marco, when following the steps from the Description I'm not seeing the preference browser.urlbar.searchSuggestionsChoice  on the updated build. 

If I do the workaround from Comment 8, I do see the preference browser.urlbar.searchSuggestionsChoice on the updated build.

Is this intended in any way?

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Simona B [:simonab ] from comment #24)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101
> Firefox/55.0
> Build ID: 20170605030204
> 
> Marco, when following the steps from the Description I'm not seeing the
> preference browser.urlbar.searchSuggestionsChoice  on the updated build.

Upgrading from which build to which other build?
If you're still using the 20th of May build, it won't set it, since I cannot travel in time :)

But the new build should notice that you don't have that pref but that you made a choice, and thus just fallback to disable suggestions.

Comment 26

[Simona Badau, Desktop QA]

(In reply to Marco Bonardo [::mak] from comment #25)
> But the new build should notice that you don't have that pref but that you
> made a choice, and thus just fallback to disable suggestions.

Indeed, if I disable the search suggestions on the build from June 1 (Build ID: 20170601030206) and update to the latest Nightly 55.0a1 (Build ID: 20170605030204) - the search suggestions remain disabled and the preference browser.urlbar.searchSuggestionsChoice is displayed in about:config (it is set to false).

Verified as fixed by updating Nighly 55 from June 1st to the latest Nightly from June 5th on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.

Comment 27

[Gabi Cheta]

Verified as fixed using Fx 54.0 Build ID: 20170605134926 on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04.

Comment 28

[Ciprian Georgiu, Desktop QA]

Changing the status flag for 54.0 to verified per comment 27. Thanks for testing!