1232651 - Search suggestions are enabled in private browsing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. With a clean profile open Firefox
2. Write "t" in the URL Bar
3. Turn on search suggestions
4. Go to Settings -> Search and set Twitter as a default search engines
5. Type something in the URL Bar (Search suggestions are displayed)
6. Go to private browsing and type "t" in URL Bar

Expected results:
- Search suggestions are not displayed in private browsing

Actual results:
- Search suggestions are displayed in private browsing

Note:
- reproducible on 45.0a2 Aurora and 46.0a1 Nightly
- on 43 Beta 10 search suggestions are not displayed in normal and private browsing after turning on search suggestions

Comment 1

[Andrzej Hunt :ahunt]

This sounds like it could have the same cause as Bug 1223983.

Comment 2

[:Margaret Leibovic]

(In reply to Andrzej Hunt :ahunt from comment #1)
> This sounds like it could have the same cause as Bug 1223983.

Tracking for 44+ to match this. If this issue doesn't affect beta, feel free to re-nom.

Comment 3

[:Margaret Leibovic]

FWIW, I also saw search suggestions in private browsing yesterday on Nightly.

Comment 4

[Andrzej Hunt :ahunt]

This turns out to be a separate bug (patch uploaded in the next comment hopefully), however I'm wondering whether we want to show

1) No suggestions at all (this is what my patch does)
or
2) Show just history suggestions

I think it would make more sense to go for no suggestions - we don't save search history in private mode, so it's potentially confusing if search history is still shown (i.e. the user may be confused as to whether searches in private browsing might be saved). (Doing this is also simpler to implement.) On the other hand we do show browsing history suggestions below the search suggestions, so showing search history would be more consistent?

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

We also remove the special case for updating mSuggestClient when
search engines are modified during private browsing - this causes
a weird edge case where we potentially show history suggestions
after switching search engines (this seems to be because having
mSuggestClient == null avoids us even reaching the stage where
we usually recycle unneeded suggestions).

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

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Andrzej Hunt :ahunt from comment #4)
> This turns out to be a separate bug (patch uploaded in the next comment
> hopefully), however I'm wondering whether we want to show
> 
> 1) No suggestions at all (this is what my patch does)
> or
> 2) Show just history suggestions
> 
> I think it would make more sense to go for no suggestions - we don't save
> search history in private mode, so it's potentially confusing if search
> history is still shown (i.e. the user may be confused as to whether searches
> in private browsing might be saved). (Doing this is also simpler to
> implement.) On the other hand we do show browsing history suggestions below
> the search suggestions, so showing search history would be more consistent?

You make some good points, :ahunt. 

I think we should lump these two together and NOT show history or suggestions. 

In a mobile context, its hard for us to safely assume this is even the same person. Phones get passed around and Private Browsing should let you feel safe when you do this.

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8704858 [details]
MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

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

Comment 8

[:Margaret Leibovic]

This missed 44... let's try to land this for 45.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8704858 [details]
MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

https://reviewboard.mozilla.org/r/29775/#review29629

If it works for you, works for me. Sorry for the delay!

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:600
(Diff revision 2)
> -        if (mSuggestClient == null || (!mSuggestionsEnabled && !mSavedSearchesEnabled)) {
> +        Tab tab = Tabs.getInstance().getSelectedTab();

nit: `final`

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:603
(Diff revision 2)
> +        if (isPrivate || mSuggestClient == null || (!mSuggestionsEnabled && !mSavedSearchesEnabled)) {

I believe `mSuggestClient == null` was used to determine if we were in private mode or not so it seems better to be explicit here.

Since this was already supposed to have worked, I assume it automatically clears the active suggestions when switching from normal browsing to private browsing, right? I can't seem to find that code...

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:680
(Diff revision 2)
>                      // Only create a new instance of SuggestClient if it hasn't been
>                      // set yet.
> -                    maybeSetSuggestClient(suggestTemplate, isPrivate);
> +                    maybeSetSuggestClient(suggestTemplate);

nit: I feel like this comment describes what happens in the method, which seems unnecessary if the method was named better.

Can we remove the comment and update the name? e.g. `initSuggestClientIfNotAlreadyInit` (I'm sure there's a better name)

Comment 10

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/87c4fbaccbf5b794144f8f56d5b74d1f44769c20
Bug 1232651 - don't show search suggestions in private browsing r=mcomella

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/75c6a6c1fec2 for robocop bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=6916943&repo=fx-team

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8704858 [details]
MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

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

Comment 13

[Andrzej Hunt :ahunt]

> 
> I believe `mSuggestClient == null` was used to determine if we were in
> private mode or not so it seems better to be explicit here.
> 
> Since this was already supposed to have worked, I assume it automatically
> clears the active suggestions when switching from normal browsing to private
> browsing, right? I can't seem to find that code...
> 

It turns out we probably do need this - mSuggestClient isn't set until we receive the search engines in setSearchEngines - if you start typing before we receive those, then mSuggestClient will be null - so we still need to do the null-check (in addition to tracking private mode separately).

Comment 14

[Andrzej Hunt :ahunt]

(In reply to Wes Kocher (:KWierso) from comment #11)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/75c6a6c1fec2
> for robocop bustage:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=6916943&repo=fx-team

Try build succeeded after fixing the patch (comment #12), therefore pushing back to fx-team!

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

Comment 15

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/557fb171e6562816f16a317c0d45f688609361c7
Bug 1232651 - don't show search suggestions in private browsing r=mcomella

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/557fb171e656

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2016-02-02-14-11-01.png

Reproduced the issue also on 45 Beta 2 using ZTE Grand x86 (Android 4.0.4)

Comment 18

[:Margaret Leibovic]

Should we uplift this? It's too late for 45, but not 46.

Comment 19

[u421692]

Verified as fixed on Aurora 47.0a2 (2016-03-09)

Comment 20

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: Verified fix and we are early in the beta cycle.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking; uplift sounds good to me too. This could make it into beta 4 early next week.

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8704858 [details]
MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Search suggestions are shown in private browsing, which is potentially confusing since we aren't retrieving suggestions then, and we also don't store searches in private browsing.
[Describe test coverage new/current, TreeHerder]: manual testing.
[Risks and why]: Low risk: we now check an additional condition when deciding whether to show suggestions. This change is already in aurora without any issues.
[String/UUID change made/needed]: none.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8704858 [details]
MozReview Request: Bug 1232651 - don't show search suggestions in private browsing r=mcomella

Verified on aurora. Shipping this faster sounds good!
This should make it into 46 for the beta 4 build early next week.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0de4101e6ef6

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Search suggestions are not enabled in private browsing, so:
Verified as fixed using:
Device: Grand X in (Android 4.0.4)
Build: Firefox for Android 46 Beta 6