Closed Bug 1124884 Opened 6 years ago Closed 6 years ago

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

Categories

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

35 Branch
ARM
Android
enhancement
Not set
normal

Tracking

(firefox36 wontfix, firefox37+ verified, firefox38+ verified, firefox39 verified, fennec37+)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox36 --- wontfix
firefox37 + verified
firefox38 + verified
firefox39 --- verified
fennec 37+ ---

People

(Reporter: josh, Assigned: AndyP, Mentored)

References

()

Details

(Whiteboard: [lang=js][lang=java])

Attachments

(1 file, 3 obsolete files)

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.
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?
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(margaret.leibovic)
Hardware: Other → ARM
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.
Mentor: margaret.leibovic
tracking-fennec: --- → ?
Flags: needinfo?(margaret.leibovic)
Whiteboard: [lang=js][lang=java]
tracking-fennec: ? → 37+
Assignee: nobody → ally
I've added the clear functionality from the clearHistory method of SearchPreferenceActivity.java to the handleClearHistory method of GeckoApp.java.
Attachment #8565742 - Flags: review?(margaret.leibovic)
I think we should add a clearSearchHistory into the DB itself, like we do for clearHistory.
Assignee: ally → drag
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?
Attachment #8565742 - Flags: review?(margaret.leibovic) → feedback+
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?
Attachment #8565742 - Attachment is obsolete: true
Attachment #8566277 - Flags: feedback?(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.
Attachment #8566277 - Flags: feedback?(margaret.leibovic) → feedback+
Just to clarify, do you want me to omit both notifications in LocalBrowserDB or only the mHistoryUriWithProfile notification?
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic)
Both notifications are omitted now.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39dcb6892a0e
Attachment #8566277 - Attachment is obsolete: true
Attachment #8567715 - Flags: review?(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.
Attachment #8567715 - Flags: review?(margaret.leibovic) → review+
Comments addressed. :)
Attachment #8567715 - Attachment is obsolete: true
Attachment #8567741 - Flags: review?(margaret.leibovic)
Comment on attachment 8567741 [details] [diff] [review]
bug1124884_clearHistoryInSearchWidget.diff -v4

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

Excellent, thanks!
Attachment #8567741 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b78da966c0ca
Keywords: checkin-needed
Whiteboard: [lang=js][lang=java] → [lang=js][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b78da966c0ca
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][lang=java][fixed-in-fx-team] → [lang=js][lang=java]
Target Milestone: --- → Firefox 39
Thanks, everyone! :)
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.
Flags: needinfo?(drag)
Keywords: verifyme
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]: -
Flags: needinfo?(drag)
Attachment #8567741 - Flags: approval-mozilla-beta?
Attachment #8567741 - Flags: approval-mozilla-aurora?
(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]: -
I would like to see this verified before uplift as well.

Aaron/Kevin - Do either of you have time to verify the fix today?
Flags: needinfo?(kbrosnan)
Flags: needinfo?(aaron.train)
Flags: needinfo?(aaron.train) → needinfo?(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).
Flags: needinfo?(flaviu.cos)
Keywords: verifyme
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+
Attachment #8567741 - Flags: approval-mozilla-beta?
Attachment #8567741 - Flags: approval-mozilla-beta+
Attachment #8567741 - Flags: approval-mozilla-aurora?
Attachment #8567741 - Flags: approval-mozilla-aurora+
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)
Also, when checking only "Browsing history" will delete the search activity and checking only "Form & search history" won't delete. Is this expected?
Flags: needinfo?(drag)
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(drag)
Flags: needinfo?(margaret.leibovic)
Verified as fixed in build 38.0a2 (2015-03-03);
Device: Motorola Razr (Android 4.1.2).
Status: RESOLVED → VERIFIED
(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.
See Also: → 1139379
(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.
Depends on: 1139379
Flags: needinfo?(kbrosnan)
Duplicate of this bug: 1140936
Duplicate of this bug: 1143585
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.