816527 - In per-window private browsing switch-to-tab should not show tabs from private windows

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Siddhartha Dugar [:sdrocking]]

Bug seen on UX hourly with PB enabled.

Comment 1

[(no longer active)]

Hmm, I'm pretty sure that I had filed this bug before, but now I can't find it.  Josh, have you seen that bug, or am I just imagining things?

Comment 2

[(no longer active)]

Apparently I have not filed that bug!

Comment 3

[(no longer active)]

Attachment: Patch (v1)

(The description of this patch comes in the commit message.)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 686979 [details] [diff] [review]
Patch (v1)

I'm not sure I understand the reasoning behind disabling switch-to-tab in private browsing windows. Isn't it enough to make sure that the private tabs aren't ever offered? What's the downside to exposing non-private tabs for easy access from private windows? Removing that would remove the need for an additional gross autocompletesearchparam hack, which would be nice :)

Similarly, you could just make switchIfURIInWindow only check aWindow's private browsing state (and return false if it's true).

Blair should probably review the adjusted patch here.

Comment 5

[(no longer active)]

(In reply to comment #4)
> I'm not sure I understand the reasoning behind disabling switch-to-tab in
> private browsing windows. Isn't it enough to make sure that the private tabs
> aren't ever offered? What's the downside to exposing non-private tabs for easy
> access from private windows? Removing that would remove the need for an
> additional gross autocompletesearchparam hack, which would be nice :)

If we offer non-private suggestions for a tab that the user is trying to open in a private window, and they accept it, they may be under the false assumption that they're in private browsing mode when they switch to that tab, and this leads to confusion at best and data leakage at worst.

> Similarly, you could just make switchIfURIInWindow only check aWindow's private
> browsing state (and return false if it's true).

I don't think that's what we want to allow.  Tab switching between private and non-private windows is very error prone, since the tab switching UI has no indication of where the target is, so the user cannot make a good decision.  We should protect them against both directions here.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 686979 [details] [diff] [review]
Patch (v1)

In that case it seems like it would be simpler to have the front-end adjust its autocompletesearchparam to not include "enable-actions" in private browsing windows, rather than the current thing where it adds a "private" flag.

The comment in switchIfURIInWindow seems wrong, I think you want s/either/neither/.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Comment on attachment 686979 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=686979
> Patch (v1)
> 
> In that case it seems like it would be simpler to have the front-end adjust its
> autocompletesearchparam to not include "enable-actions" in private browsing
> windows, rather than the current thing where it adds a "private" flag.

OK, I can do that, it's a very easy change to make, and make the patch a bit cleaner.

> The comment in switchIfURIInWindow seems wrong, I think you want
> s/either/neither/.

Right, will fix it.

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Comment 9

[(no longer active)]

Comment on attachment 687248 [details] [diff] [review]
Patch (v2)

Blair is out this week.  Is there anyone else who can review this patch?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 687248 [details] [diff] [review]
Patch (v2)

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+                if (!isBlankPageURL(aLocation.spec)
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+                    // Don't find this tab when finding a switch to tab candidate.
>+                    && !PrivateBrowsingUtils.isWindowPrivate(window)

I'd rephrase the comment:
"Tabs in private windows aren't registered as "Open" so that they don't appear as switch-to-tab candidates."

>diff --git a/browser/base/content/test/browser_bug816527.js b/browser/base/content/test/browser_bug816527.js

This test is a bit hard to follow. Why is there a TabClose handler and an onload handler? Finding a way to avoid the setTimeout would also be good, but I haven't thought about it too much.

Comment 11

[(no longer active)]

Attachment: Patch (v3)

This patch fixes the comment, and adds an extensive comment to the test explaining what you were asking about.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The "aExpectSuccess" bit was confusing me. Since no callers pass true, can you just remove it?

Comment 13

[(no longer active)]

Attachment: Patch (v4)

Oh, right!  Done.  Sorry about that, I should've removed it before.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57be4b233874

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57be4b233874

Comment 16

[[:Aleksej]]

2013-01-04-03-08-23-mozilla-central-firefox-20.0a1.en-US.linux-x86_64
Private Browsing windows seem to have no switch-to-tab; and the private windows’ tabs do not show up in normal windows’ switch-to-tab.

Comment 17

[Ioana (away)]

Verified as fixed on Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130115 Firefox/21.0 and Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130115 Firefox/20.0.