1124884 - Search history is not cleared in Firefox Search after using "Clear Private Data" in Firefox

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, enhancement)

Description

[josh]

User Agent: Mozilla/5.0 (Android; Tablet; rv:36.0) Gecko/36.0 Firefox/36.0
Build ID: 20150120155239

Steps to reproduce:

1 Run some searches in Fx. Open the Fx Search app and notice that these searches are shown in the history
2 Use Settings > Privacy > Clear Private Data > Clear now to wipe form and search history
3 Open the Fx Search app


Actual results:

Searches made before using "clear private data" are displayed in Fx Search app.


Expected results:

Search history in Fx Search should be cleared when "clear private data" is used. Users have the expectation that their search history, which is added to Fx Search automatically, should also be removed from Fx Search automatically.

Comment 1

[Aaron Train [:aaronmt]]

Sounds like a legitimate request. Margaret, I could have sworn this was mentioned in another bug or was it just this bug? Should this track?

Comment 2

[:Margaret Leibovic]

I don't see another bug. But yes, we should definitely do this.

To fix this, we should send a message to Java from Sanitizer.jsm, similarly to what we do for browsing history:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#139

Then in Java, we should handle this message like this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#630

And clear search history similarly to what we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java#87

We should probably consider putting this clear search history logic in a common place that we can share.

Comment 3

[Andy Pusch [:AndyP]]

Attachment: bug1124884_clearHistoryInSearchWidget.diff

I've added the clear functionality from the clearHistory method of SearchPreferenceActivity.java to the handleClearHistory method of GeckoApp.java.

Comment 4

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

I think we should add a clearSearchHistory into the DB itself, like we do for clearHistory.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8565742 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff

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

This is looking good, but I agree with mfinkle that we should encapsulate this logic in BrowserDB.

It looks like this is actually the only place we ever call BrowserDB.clearHistory(), so I would be fine with us adding this search history clearing into that implementation:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#719

You should just add a new URI for the search history provider.

::: mobile/android/base/GeckoApp.java
@@ +536,5 @@
> +                final int numDeleted = getContentResolver().delete(
> +                    BrowserContract.SearchHistory.CONTENT_URI, null, null);
> +                if (numDeleted >= 0) {
> +                    getContentResolver().notifyChange(
> +                        BrowserContract.SearchHistory.CONTENT_URI, null);

Why do we need to notify this change, but we don't notify for the clear history call?

Comment 6

[Andy Pusch [:AndyP]]

Attachment: bug1124884_clearHistoryInSearchWidget.diff -v2

I've moved search history clearing into LocalBrowserDB.java.

(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 8565742 [details] [diff] [review]
> Why do we need to notify this change, but we don't notify for the clear
> history call?

I did a quick test without the notification in both GeckoApp.java and SearchPreferenceActivity.java and did not notice any problems. However there could still be registered observers that do stuff in the background if the notification arrives, I'm not sure about it. I'd suggest to notify the change for both cr.delete() calls even if there is no observer registered as of now. What do you think?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8566277 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff -v2

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

This is looking good, but let's get rid of the notifyChange bits, since it's outside the scope of this bug.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +724,5 @@
> +        int numDeleted = cr.delete(mHistoryUriWithProfile, null, null);
> +        if (numDeleted >= 0) {
> +            cr..notifyChange(mHistoryUriWithProfile, null);
> +        } else {
> +            Log.e(LOGTAG, "Error clearing browser history.");

I would prefer to omit this notification in this patch, and we can file a follow-up bug to add it if it's necessary.

We should try to keep the scope of this patch here as small as possible so that there will less risk of causing a regression, since we would like to uplift this to beta.

Comment 8

[Andy Pusch [:AndyP]]

Just to clarify, do you want me to omit both notifications in LocalBrowserDB or only the mHistoryUriWithProfile notification?

Comment 9

[:Margaret Leibovic]

(In reply to Andy Pusch [:AndyP] from comment #8)
> Just to clarify, do you want me to omit both notifications in LocalBrowserDB
> or only the mHistoryUriWithProfile notification?

I think you should omit both.

Comment 10

[Andy Pusch [:AndyP]]

Attachment: bug1124884_clearHistoryInSearchWidget.diff

Both notifications are omitted now.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39dcb6892a0e

Comment 11

[:Margaret Leibovic]

Comment on attachment 8567715 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff

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

Looking good! r+ with comments addressed.

::: mobile/android/base/GeckoApp.java
@@ +391,5 @@
>                  // Prepare the panel every time before showing the menu.
>                  onPreparePanel(featureId, mMenuPanel, mMenu);
>              }
>  
> +            return mMenuPanel;

Please remove these unrelated whitepace changes from your patch. They clearly don't change the logic, but they affect hg history, so I think we should leave them out if we're not touching neighboring code.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +98,5 @@
>      private final Uri mCombinedUriWithProfile;
>      private final Uri mUpdateHistoryUriWithProfile;
>      private final Uri mFaviconsUriWithProfile;
>      private final Uri mThumbnailsUriWithProfile;
> +    private final Uri mSearchWidgetHistoryUri;

Please update this to be mSearchHistoryUri.

We store all search history, both what you type in the urlbar and what you type in the search activity, so including the term "widget" in here is misleading.

Comment 12

[Andy Pusch [:AndyP]]

Attachment: bug1124884_clearHistoryInSearchWidget.diff -v4

Comments addressed. :)

Comment 13

[:Margaret Leibovic]

Comment on attachment 8567741 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff -v4

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

Excellent, thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/b78da966c0ca

Comment 16

[josh]

Thanks, everyone! :)

Comment 17

[:Margaret Leibovic]

Andy, would you mind filling out a request to uplift this to aurora and beta? You can do that by flipping the approval-mozilla-aurora? and approval-mozilla-beta? flags in the patch attachment.

However, I would also like to see QA verify this on Nnghtly before we land the patches on aurora/beta.

Comment 18

[Andy Pusch [:AndyP]]

Comment on attachment 8567741 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff -v4

Approval Request Comment
[Feature/regressing bug #]: Feature
[User impact if declined]: Searches made before using "clear private data" are still displayed.
[Describe test coverage new/current, TreeHerder]: Tested on my Android Phone, 39dcb6892a0e
[Risks and why]: Very low risk of causing some other problem when clearing data.
[String/UUID change made/needed]: -

Comment 19

[:Margaret Leibovic]

(In reply to Andy Pusch [:AndyP] from comment #18)

> Approval Request Comment
> [Feature/regressing bug #]: Feature

To elaborate a bit here, this was introduced as part of the search activity feature, which we shipped in Firefox 35.

> [User impact if declined]: Searches made before using "clear private data"
> are still displayed.
> [Describe test coverage new/current, TreeHerder]: Tested on my Android
> Phone, 39dcb6892a0e
> [Risks and why]: Very low risk of causing some other problem when clearing
> data.

I agree this patch is safe to uplift, but it does add additional logic to the "clear private data" action, hence the (tiny) risk.

> [String/UUID change made/needed]: -

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

I would like to see this verified before uplift as well.

Aaron/Kevin - Do either of you have time to verify the fix today?

Comment 21

[Flaviu Cos]

Verified as fixed in build 39.0a1 (2015-03-01);
Devices: 
Asus Transformer Tab (Android 4.2.1);
Samsung Galaxy R (Android 2.3.4).

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567741 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff -v4

Now that this has been verified, let's get it into Beta 2. Beta+ Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5c0c7cd2cc66

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12b0612ba016

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Going to Settings -> Privacy -> Clear Private Data -> Clear now-> Check all options -> Clear data, all search history is cleared from search activity, so:
Verified as fixed on 37 beta 2
Device: Sony Zperia Z2 (Android 4.4.4)

Comment 26

[Teodora Vermesan (:TeoVermesan)]

Also, when checking only "Browsing history" will delete the search activity and checking only "Form & search history" won't delete. Is this expected?

Comment 27

[Andy Pusch [:AndyP]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #26)
> Also, when checking only "Browsing history" will delete the search activity
> and checking only "Form & search history" won't delete. Is this expected?

Yes this is expected, I've discussed this with Margaret in IRC. I think the name of that field is a bit misleading though.

Comment 28

[Flaviu Cos]

Verified as fixed in build 38.0a2 (2015-03-03);
Device: Motorola Razr (Android 4.1.2).

Comment 29

[:Margaret Leibovic]

(In reply to Andy Pusch [:AndyP] from comment #27)
> (In reply to Teodora Vermesan (:TeoVermesan) from comment #26)
> > Also, when checking only "Browsing history" will delete the search activity
> > and checking only "Form & search history" won't delete. Is this expected?
> 
> Yes this is expected, I've discussed this with Margaret in IRC. I think the
> name of that field is a bit misleading though.

I agree. I feel like that should probably just be "Form history". We could file a bug about that.

Comment 30

[Flaviu Cos]

(In reply to :Margaret Leibovic from comment #29)
> 
> I agree. I feel like that should probably just be "Form history". We could
> file a bug about that.

Logged Bug 1139379 for that.