1048444 - Search activity displays private browsing searches from browser

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[Demetrius S.]

User Agent: Mozilla/5.0 (Android; Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 (Nightly/Aurora)
Build ID: 20140803030203

Steps to reproduce:

Opened a private tab and searched for something


Actual results:

Opened the nightly search app and found my private searches in the history


Expected results:

Private tab searches shouldn't be found in the history of the nightly search apo

Comment 1

[:Margaret Leibovic]

This is no good! This was introduced by bug 1038227.

We won't be showing these search terms anywhere in Firefox 33, but we should still uplift a fix.

Comment 2

[:Margaret Leibovic]

This is important, let's get a fix sooner rather than later.

Comment 3

[:Margaret Leibovic]

I can take this.

Comment 4

[:Margaret Leibovic]

Guest browsing shouldn't be an issue here because searches would be stored to the guest profile, and we only read search terms from the default profile in the search activity.

Comment 5

[:Margaret Leibovic]

Attachment: Don't store searches that happen in private browsing mode

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8472553 [details] [diff] [review]
Don't store searches that happen in private browsing mode

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

::: mobile/android/base/BrowserApp.java
@@ +1826,5 @@
>       *        a search query to store. We won't store empty queries.
>       */
>      private void storeSearchQuery(String query) {
> +        // Don't store searches that happen in private tabs. This assumes the user can only
> +        // perform a search inside the currently selected tab.

This assumption is technically wrong, and it might or might not matter in practice.

There are three sources of calls for this method:

* BrowserApp.onSearch, from SearchEngineRow; this is safe.
* BrowserApp.handleMessage, via EventDispatcher; this is not quite safe. It's possible for the user to change tabs before the event gets processed.
* BrowserApp.commitEditingMode, which spawns a Runnable on the background thread to record a barkeyword search. This is definitely not safe; the background thread can block for arbitrary amounts of time.

In practice I doubt this is a concern; people don't use keyword searches much. The right solution is for BrowserApp.recordSearch/commitEditingMode, and JS-side message senders, to not send (undecorated) Search: messages -- not only will that avoid this kind of race, but it'll also stop us storing these searches in FHR, if we're doing that.

Take a look, see if it's easy to be safer. If not, then r+.

Comment 7

[:Margaret Leibovic]

Attachment: (alternate) Don't store searches that happen in private browsing mode (

This aims to address the risky points that rnewman raised in his last comment by dropping the query term in JS.

This still makes an assumption that you aren't loading the search in a background tab. We're already making that assumption for the userSearch property, so I'm hoping that's reasonable, but I don't always trust our past selves :)

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8472647 [details] [diff] [review]
(alternate)  Don't store searches that happen in private browsing mode (

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

Hopefully nsDefaultURIFixup dispatches the keyword-search event synchronously (it looks like it does); otherwise, the "change tabs before the event gets processed" race would still apply.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/7aef335d6627

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7aef335d6627

Comment 11

[:Margaret Leibovic]

Comment on attachment 8472647 [details] [diff] [review]
(alternate)  Don't store searches that happen in private browsing mode (

Approval Request Comment
[Feature/regressing bug #]: bug 1038227
[User impact if declined]: private browsing search terms are stored and can appear in search activity
[Describe test coverage new/current, TBPL]: no automated tests, tested locally and landed on Nightly
[Risks and why]: low-risk, adds checks around calls to store search terms to make sure we're not in private browsing mode
[String/UUID change made/needed]: n/a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb429fd7d9c2