Closed Bug 1035439 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox32 --- unaffected
firefox33 --- verified
fennec 33+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(4 files, 2 obsolete files)

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).
Ian, what do you want to do here?
Flags: needinfo?(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.
Flags: needinfo?(ibarlow)
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.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #8455454 - Flags: review?(margaret.leibovic)
Makes the recent tabs panel show private tabs in private browsing and normal tabs in normal browsing.
Attachment #8455456 - Flags: review?(margaret.leibovic)
More patches incoming shortly...
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.
Attachment #8455454 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8455591 - Flags: review?(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?
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.
Attachment #8455605 - Flags: review?(margaret.leibovic)
(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.
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.
Attachment #8455456 - Attachment is obsolete: true
Attachment #8455456 - Flags: review?(margaret.leibovic)
Attachment #8455666 - Flags: review?(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.
Attachment #8455666 - Flags: review?(margaret.leibovic) → review+
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!
Attachment #8455591 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8455605 - Flags: review?(margaret.leibovic)
Errors fixed.
Attachment #8455605 - Attachment is obsolete: true
Attachment #8455733 - Flags: review?(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?
Attachment #8455733 - Flags: review?(margaret.leibovic) → review+
(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.
(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?
(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.
Depends on: 1039471
tracking-fennec: ? → 33+
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)
If that's the correct behaviour, then we can mark this verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.