1216826 - Search suggestions should not be enabled in private browsing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Search suggestions aren't supposed to be shown in private browsing (see bug 965321 for some context). Regression from recent search work?

Comment 1

[:Margaret Leibovic]

Mike or Sergej, do you know what's going on here?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

This looks like regression from bug 1186683.

mSuggestClient is set in setSearchEngines if the current tab is not private [1]. mSuggestClient is then used in the rest of the class to get the search suggestions provided it's not null, in which case it wouldn't retrieve the suggestions. However, mSuggestClient only gets updated when we set the search engines (which used to be every time BrowserSearch was created and thus shown) – we should be sure to update it every time BrowserSearch is shown.

Watch out for bug 1206628 (the patch has not landed yet but is r+) which only calls all of setSearchEngines when there are new search engines.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java?rev=b34903a7c80c#647

Comment 3

[Sergej Kravcenko]

Are we taking about suggestions from engine or from history? Now suggestions history is always visible. Not sure that this is correct. 
Suggestions history save is ignored in private mode.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

via irc, Prabhjyot is on this one! Thanks!

Comment 5

[Allison Naaktgeboren :ally]

hey psd, mcomella tells me you mentioned having a patch yesterday for this on irc, but I don't see one. How's this going? Are you stuck on anything?

Comment 6

[Prabhjyot Sodhi [:psd]]

Hi!

In the patch for bug 1207340 (which worked but was plagued with a flicker), I just added a couple of lines of code, to change the mSuggestClient each time, setSearchEngines was called. This would update mSuggestClient to be null for private tabs and hence remove the prompt at the end.

Now, as soon as we can remove the flicker problem from bug 1207340, I should be able to produce a patch for this, or maybe even fix this in bug 1207340 itself.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

This is fixed by the patch in bug 1207340, right? If so, can you dupe this bug to that one?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I notice we also have two individual search suggestion preferences now – one for network suggestions and one for search history. Can you be sure to check that both work correctly?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #8)
> I notice we also have two individual search suggestion preferences now – one
> for network suggestions and one for search history. Can you be sure to check
> that both work correctly?

I'm assuming that:
1. Suggestions from search engine _should not_ be enabled in private browsing
2. Suggestions from search history _should_ be enabled in private browsing

#2 would only use any existing search history, which was saved in non private browsing sessions. Search history is not saved in private browsing so any search terms used in a private browsing session could not appear as suggestions.

Comment 10

[Prabhjyot Sodhi [:psd]]

Attachment: 1216826.patch

So, patch for bug 1207340 did not completely solve this.
But this seems to! :)

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

I assume that the loader will be around if we switch to a normal tab?

Comment 12

[Prabhjyot Sodhi [:psd]]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> I assume that the loader will be around if we switch to a normal tab?
Yes! I have tested it. Jumping between Private and non private tabs, suggestions work when they should.:)

Comment 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

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

Cool and clean solution!

I'm slightly concerned about the efficiency vs. re-using the existing Loader, but we don't do an excessive number of allocations and hopefully neither does the framework so this wfm. I don't notice perf issues locally either.

btw, sorry for the delay! I should have gotten to this before the holiday.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f703469102

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e6dbd0ee282

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e6dbd0ee282

Comment 17

[Prabhjyot Sodhi [:psd]]

Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1186683
[User impact if declined]: User shown suggestions in private browsing, High impact
[Describe test coverage new/current, TreeHerder]: Testing Manually, visual testing required
[Risks and why]: Low risk - simple change in loader of suggestions 
[String/UUID change made/needed]: None

Comment 18

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

Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

Since this is a bad regression, taking it to Aurora44 soon makes sense.

Comment 19

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

Ioana, would you be able to verify this fixes the issue as expected? Also a bit of focused testing around search suggestions and search history in regular and private browsing mode will be appreciated as well.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04630b5647aa

Comment 21

[Ioana Chiorean]

(In reply to Ritu Kothari (:ritu) from comment #19)
> Ioana, would you be able to verify this fixes the issue as expected? Also a
> bit of focused testing around search suggestions and search history in
> regular and private browsing mode will be appreciated as well.

Yes - I am currently in PTO so I will ask someone from the team to look it up.

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Nexus 4 (Android 5.1.1)
Build: Firefox for Android 45.0a1 (2015-
In normal browsing type some letters in URL Bar and enable search suggestions. Go to private browsing and start typing some letters. Search suggestions are not displayed.

While testing this, I found a new issue. Aurora and Nightly are affected. I filled Bug 1230568: In normal browsing start typing in the URL Bar ("br"). Don't choose to turn on/off search suggestions. Go to private browsing and start typing in the URL Bar ("br"). See that no search suggestions are displayed. Now go back to normal browsing and start typing in the URL Bar ("br"). "would you like to turn on search suggestions" message should be displayed. But the message doesn't appear. Only "br" is displayed. As if the user chose to turn off search suggestions.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/04630b5647aa

Comment 24

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

(In reply to Teodora Vermesan (:TeoVermesan) from comment #22)
> Tested using:
> Device: Nexus 4 (Android 5.1.1)
> Build: Firefox for Android 45.0a1 (2015-
> In normal browsing type some letters in URL Bar and enable search
> suggestions. Go to private browsing and start typing some letters. Search
> suggestions are not displayed.
> 
> While testing this, I found a new issue. Aurora and Nightly are affected. I
> filled Bug 1230568: In normal browsing start typing in the URL Bar ("br").
> Don't choose to turn on/off search suggestions. Go to private browsing and
> start typing in the URL Bar ("br"). See that no search suggestions are
> displayed. Now go back to normal browsing and start typing in the URL Bar
> ("br"). "would you like to turn on search suggestions" message should be
> displayed. But the message doesn't appear. Only "br" is displayed. As if the
> user chose to turn off search suggestions.

Thanks for the verification and catching this regression. Great work!

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Also I've filled Bug 1232651. On latest Aurora and Nightly - Search suggestions are displayed in private browsing for Twitter default search engine. On 43 Beta 10 search suggestions are not displayed in normal and private browsing after turning on search suggestions.