1035439 - Opening a tab from Recent Tabs panel doesn't work in private browsing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open a normal tab and close it.
2) Create a new private tab.
3) Click the URL bar and swipe to Recent Tabs panel.
4) Click the closed tab from step 1.

After these STR, the closed tab is opened, but it's not selected. We should probably do one of the following:
* Keep this same behavior, but switch to the tab after opening it.
* Don't show the Recent Tabs panel in private browsing.
* Show the Recent Tabs panel, but show only recently closed private tabs in the private browsing Recent Tabs panel, and show only normal tabs in the normal Recent Tabs panel (i.e., an alternate fix to bug 1030757).

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Ian, what do you want to do here?

Comment 2

[Ian Barlow (:ibarlow)]

I think we should create the tabs in private browsing -- keep users in their current context. That's also more consistent with what we do with other panels (history, top sites etc) when you're in private browsing.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Clear private undo close tab data when leaving private browsing

When the user leaves private browsing, all in-memory private browsing data is wiped. We should do the same for closed tabs.

This step is also important to prevent private tabs from your previous private session from appearing in your new private session.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Show closed tabs of the current private state in RecentTabsPanel

Makes the recent tabs panel show private tabs in private browsing and normal tabs in normal browsing.

Comment 5

[Brian Nicholson (:bnicholson)]

More patches incoming shortly...

Comment 6

[:Margaret Leibovic]

Comment on attachment 8455454 [details] [diff] [review]
Part 1: Clear private undo close tab data when leaving private browsing

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

::: mobile/android/components/SessionStore.js
@@ +171,5 @@
>          this._notifyClosedTabs = false;
>          break;
> +      case "last-pb-context-exited":
> +        // Clear private closed tab data when we leave private browsing.
> +        for ([, window] in Iterator(this._windows)) {

You should declare this local variable with let, to avoid it leaking out.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Use selected tab private state in openUrlAndStopEditing

Tabs opened through the RecentTabsPanel are opened via openUrlAndStopEditing(). This method is currently non-PB aware, meaning tabs opened are always non-private.

We could overload openUrlAndStopEditing and/or add a flags parameter, but I think it's safe to just make this decision based on the selected tab's private state.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8455456 [details] [diff] [review]
Part 2: Show closed tabs of the current private state in RecentTabsPanel

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

::: mobile/android/base/home/RecentTabsPanel.java
@@ +240,5 @@
> +        final ArrayList<ClosedTab> closedTabs = new ArrayList<ClosedTab>();
> +        for (NativeJSObject tab : tabs) {
> +            if (tab.getBoolean("isPrivate") == isPrivate) {
> +                closedTabs.add(new ClosedTab(tab.getString("url"), tab.getString("title")));
> +            }

How often is this refreshed? Does this work properly if you switch between a normal about:home tab and a private browsing about:home tab?

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Change undoCloseTab to accept closed tab data

The first patches introduce a tricky problem: we clear private data when we leave private browsing mode, but we require the private data for the undo close tab toast to work.

I was a bit torn on how to handle this, but I ended up forwarding the closed tab data from the toast callback to SessionStore. So rather than undoing a closed tab by index, this uses the closed tab data handed directly to undoCloseTab, meaning we can undo a closed tab even if the data isn't in the closedTabs array on the window.

Other options considered:
* Don't show the toast for the last private tab closed. It sort of makes sense since closing the last private tab is the trigger for leaving PB, but it's not an ideal UX.
* We could clear the private data at the *beginning* of private browsing mode, but this violates the private browsing rules that your data is cleared once PB is closed (since we hold onto it until the next PB session).
* We could wait until the undo close tab toast disappears before clearing this data, but it would require adding a hidden callback to the toast API, which seems heavy.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #6)
> You should declare this local variable with let, to avoid it leaking out.

Thanks, nice catch.

(In reply to :Margaret Leibovic from comment #8)
> How often is this refreshed? Does this work properly if you switch between a
> normal about:home tab and a private browsing about:home tab?

Good point -- it works in the majority of cases since people will usually open the Home view to see the recent tabs list while in private browsing, but I didn't consider someone manually navigating to about:home during PB.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Show closed tabs of the current private state in RecentTabsPanel, v2

Different approach that filters the private tabs before sending them to Java. This keeps RecentTabsPanel agnostic about tabs' private states, and also avoids unnecessary JSON stringification/parsing for tabs that would be ignored.

I also removed the "mClosedTabs.length > 0 && canLoad()" condition, which seemed wrong:
* If mClosedTabs.length is 0, we still want to refresh (otherwise we would show stale tabs if the closedTabs list was cleared).
* We also still want to refresh if canLoad() is false, which can happen if RecentTabsPanel is created, but not the current panel.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8455666 [details] [diff] [review]
Part 2: Show closed tabs of the current private state in RecentTabsPanel, v2

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

Looks good, I think this is a better approach.

::: mobile/android/base/home/RecentTabsPanel.java
@@ -251,5 @@
>              public void run() {
>                  mClosedTabs = closedTabs;
>  
>                  // Reload the cursor to show recently closed tabs.
> -                if (mClosedTabs.length > 0 && canLoad()) {

I can only imagine I added this to avoid restarting the loader if there were no closed tabs, but that's flawed logic, since we would want to clear out the recent tabs list if there used to be closed tabs and now there are none.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8455591 [details] [diff] [review]
Part 3: Use selected tab private state in openUrlAndStopEditing

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

Yeah, I think this is a fine assumption for now. This should change when we fix bug 732752. I should get back to that!

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8455605 [details] [diff] [review]
Part 4: Change undoCloseTab to accept closed tab data

Sorry, this patch is busted. Made some last minute changes I didn't test! Posting updated patch shortly.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Change undoCloseTab to accept closed tab data, v2

Errors fixed.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8455733 [details] [diff] [review]
Part 4: Change undoCloseTab to accept closed tab data, v2

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

Looks good, I'm glad you didn't need to pass stuff along to Java.

::: mobile/android/chrome/content/browser.js
@@ +1046,5 @@
>        });
>      }
> +
> +    aTab.destroy();
> +    this._tabs.splice(tabIndex, 1);

Why did you decide to move this down here?

Comment 17

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #16)
> Why did you decide to move this down here?

When we call aTab.destroy() on the last private tab, it ends up firing the last-pb-context-exited event, which clears the private tab data in SessionStore. And if the private tab data is cleared, closedTabData will be null in the undo toast block below.

Comment 18

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/35cfb1f02918
https://hg.mozilla.org/integration/fx-team/rev/81c71f117c6a
https://hg.mozilla.org/integration/fx-team/rev/36cb524c546a
https://hg.mozilla.org/integration/fx-team/rev/420209cd2a0a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35cfb1f02918
https://hg.mozilla.org/mozilla-central/rev/81c71f117c6a
https://hg.mozilla.org/mozilla-central/rev/36cb524c546a
https://hg.mozilla.org/mozilla-central/rev/420209cd2a0a

Comment 20

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > Why did you decide to move this down here?
> 
> When we call aTab.destroy() on the last private tab, it ends up firing the
> last-pb-context-exited event, which clears the private tab data in
> SessionStore. And if the private tab data is cleared, closedTabData will be
> null in the undo toast block below.

But don't we already store closedTabData in a local variable? So shouldn't that be unaffected?

Comment 21

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #20)
> But don't we already store closedTabData in a local variable?

Yes, that's the closedTabData I was referring to in comment 18: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=420209cd2a0a#1026

Before, aTab.destroy() was above this line. If we destroy the tab first, the private data would be cleared before we set closedTabData.

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Based on the steps from the description:
1) Open a normal tab and close it.
2) Create a new private tab.
3) Click the URL bar and swipe to Recent Tabs panel.

After step 3 the Recent Tabs is empty.
Build: Firefox for Android 33.0a1 (2014-07-20)

Comment 23

[Aaron Train [:aaronmt]]

If that's the correct behaviour, then we can mark this verified.