Closed Bug 1216826 Opened 4 years ago Closed 4 years ago

Search suggestions should not be enabled in private browsing

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 + fixed
firefox45 --- fixed
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: bnicholson, Assigned: psd)

References

Details

(Keywords: regression)

Attachments

(1 file)

Search suggestions aren't supposed to be shown in private browsing (see bug 965321 for some context). Regression from recent search work?
Keywords: regression
Mike or Sergej, do you know what's going on here?
Flags: needinfo?(sergej)
Flags: needinfo?(michael.l.comella)
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
Assignee: nobody → michael.l.comella
Blocks: 1186683
Flags: needinfo?(sergej)
Flags: needinfo?(michael.l.comella)
tracking-fennec: ? → 44+
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.
via irc, Prabhjyot is on this one! Thanks!
Assignee: michael.l.comella → prabhjyotsingh95
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?
Flags: needinfo?(prabhjyotsingh95)
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.
Flags: needinfo?(prabhjyotsingh95)
This is fixed by the patch in bug 1207340, right? If so, can you dupe this bug to that one?
Flags: needinfo?(prabhjyotsingh95)
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?
(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.
Attached patch 1216826.patchSplinter Review
So, patch for bug 1207340 did not completely solve this.
But this seems to! :)
Flags: needinfo?(prabhjyotsingh95)
Attachment #8691047 - Flags: review?(michael.l.comella)
Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

I assume that the loader will be around if we switch to a normal tab?
(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 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.
Attachment #8691047 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e6dbd0ee282
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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
Attachment #8691047 - Flags: approval-mozilla-aurora?
Comment on attachment 8691047 [details] [diff] [review]
1216826.patch

Since this is a bad regression, taking it to Aurora44 soon makes sense.
Attachment #8691047 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(chiorean.ioana)
(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.
Flags: needinfo?(chiorean.ioana) → needinfo?(fennec)
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.
(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!
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.
Flags: needinfo?(fennec)
You need to log in before you can comment on or make changes to this bug.