1031273 - "Clear Private Data" does not clear "Recently closed tabs" from Recent Tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Device: Samsung Galaxy Nexus
OS: Android 4.2.2
Build: Firefox for Android 33.0a1 (2014-06-26)

Steps to reproduce:
1. Open a few tabs and close them (the tabs now appear in the "Recently closed tabs" list from Recent Tabs
2. Go to Settings -> Privacy -> Clear Private Data (with all options checked)
3. Go to Recent Tab panel

Expected results:
- The list should be empty

Actual results:
- The tabs closed at step 1 are still displayed in the "Recently closed tabs" list

Comment 1

[Aaron Train [:aaronmt]]

I was thinking of filing this the other day, but I think this is intentional (maybe? maybe not?). The tabs are cleared regardless on next browser startup if you opt to not restore them.

Comment 2

[:Margaret Leibovic]

We probably just need to update the sanitize/session store code to handle this case. Or to notify the recent tabs panel properly when the closed tabs list is cleared. Without looking at the code, I suspect this is caused by a missing notification.

Comment 3

[Brian Nicholson (:bnicholson)]

This is probably caused by the "mClosedTabs.length > 0" condition in RecentTabsPanel; see https://bugzilla.mozilla.org/show_bug.cgi?id=1035439#c11. If so, this should be fixed by bug 1035439.

Comment 4

[Aaron Train [:aaronmt]]

I can still reproduce this on Nightly 07/16. The list is only sanitized after a clear if I close the active about:home tab and visit it again in a new one.

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: Add handling for Sanitize:ClearHistory

This doesn't handle clearing Recent Tabs if you click the "Clear History" button from the History panel - it's not in the scope of this bug, but it seems like we'd like to do that too.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8480961 [details] [diff] [review]
Patch: Add handling for Sanitize:ClearHistory

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

::: mobile/android/base/home/RecentTabsPanel.java
@@ +247,5 @@
> +                    }
> +                }
> +            });
> +        } else if ("Sanitize:ClearHistory".equals(event)) {
> +            getLoaderManager().restartLoader(LOADER_ID_RECENT_TABS, null, mCursorLoaderCallbacks);

Does this properly update recently closed tabs as well as tabs from last time? You may need to clear out mClosedTabs as well. Although actually, shouldn't SessionStore.js sending us an updated set of tabs here with a "ClosedTabs:Data" message? Did you look into what messages we're getting from SessionStore.js? Looking into this right now, I think I found the bug.

I think the real fix is to just make sure we call _sendClosedTabsToJava in here (surrounded by a |if (this._notifyClosedTabs)| check):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#109

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Patch: Clear recent tabs

Good call! This does clear recent tabs as well as tabs from last time (which is what I was checking).

Comment 8

[:Margaret Leibovic]

Comment on attachment 8480981 [details] [diff] [review]
Patch: Clear recent tabs

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

r+ with comment addressed. Thanks!

::: mobile/android/components/SessionStore.js
@@ +116,5 @@
>            this.saveState();
>          }
>  
>          Services.obs.notifyObservers(null, "sessionstore-state-purge-complete", "");
> +        this._sendClosedTabsToJava(Services.wm.getMostRecentWindow("navigator:browser"));

You should wrap this in an |if (this._notifyClosedTabs)| check.

I think this should also fix the clear history button case. But if it doesn't we can file a follow-up for that.

Comment 9

[Chenxia Liu [:liuche]]

Derp, I just forgot...

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Patch: Clear recent tabs v2

Carrying over r+ (for aurora patch).

Approval Request Comment
[Feature/regressing bug #]: Recent Tabs home panel
[User impact if declined]: Users won't see the correct state of Recent Tabs after clearing private data until opening a new instance of the home panel
[Describe test coverage new/current, TBPL]: local testing
[Risks and why]: very low, adding a callback call that was omitted in the original patch
[String/UUID change made/needed]: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b3b162ff01c

Comment 13

[Teodora Vermesan (:TeoVermesan)]

The list is empty after clearing private data, so verified as fixed on:
Builds: Firefox for Android 34.0a1 (2014-08-31) and Firefox for Android 33.0a2 (2014-08-31)
Device: Alcatel One Touch (Android 4.1.2)