Closed Bug 1368477 Opened 3 years ago Closed 3 years ago

Disabled search suggestions are being enabled after update

Categories

(Firefox :: Address Bar, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- fixed
firefox53 --- unaffected
firefox54 --- verified
firefox55 --- verified

People

(Reporter: simona.marcu, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

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
(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.
Flags: needinfo?(simona.marcu)
Also, did you by chance reuse a profile that was already used on a recent Nightly when installing the older version?
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.
adding this to tracking, just in case.
Component: Search → Location Bar
Priority: -- → P1
Whiteboard: [fxsearch]
(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).
Flags: needinfo?(simona.marcu)
Attached video EnabledSuggestions.mp4
Marco, please see the screencast for more details.
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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 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?
Attachment #8872892 - Flags: review?(past) → review+
(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?
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/d29a7708e357
Disable search suggestions if in doubt regarding user choice from the previous version. r=past
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.
Flags: needinfo?(gchang)
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
Attachment #8872942 - Flags: review?(past)
Attachment #8872942 - Flags: approval-mozilla-beta?
Attachment #8872942 - Flags: review?(past) → review+
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.
Flags: needinfo?(gchang)
Attachment #8872942 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8872942 - Flags: approval-mozilla-esr52?
https://hg.mozilla.org/mozilla-central/rev/d29a7708e357
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify+
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
QA Whiteboard: [bugday-20170607]
Comment on attachment 8872942 [details] [diff] [review]
Beta one-liner fix

This patch is needed for bug 1364002. ESR52+
Attachment #8872942 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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?
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
(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.
Status: RESOLVED → VERIFIED
Verified as fixed using Fx 54.0 Build ID: 20170605134926 on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04.
Changing the status flag for 54.0 to verified per comment 27. Thanks for testing!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.