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

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: sdrocking, Assigned: Ehsan)

Tracking

(Depends on 2 bugs)

unspecified
Firefox 20
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20130104])

Attachments

(1 attachment, 3 obsolete attachments)

Bug seen on UX hourly with PB enabled.
Blocks: PBnGen
Blocks: fxPBnGen
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Apparently I have not filed that bug!
Posted patch Patch (v1) (obsolete) — Splinter Review
(The description of this patch comes in the commit message.)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #686979 - Flags: review?(gavin.sharp)
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.
Attachment #686979 - Flags: review?(gavin.sharp)
(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.
Attachment #686979 - Flags: review?(bmcbride)
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/.
(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.
Posted patch Patch (v2) (obsolete) — Splinter Review
Attachment #687248 - Flags: review?(bmcbride)
Attachment #686979 - Attachment is obsolete: true
Attachment #686979 - Flags: review?(bmcbride)
Comment on attachment 687248 [details] [diff] [review]
Patch (v2)

Blair is out this week.  Is there anyone else who can review this patch?
Attachment #687248 - Flags: review?(gavin.sharp)
Attachment #687248 - Flags: review?(dolske)
Attachment #687248 - Flags: review?(bmcbride)
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.
Posted patch Patch (v3) (obsolete) — Splinter Review
This patch fixes the comment, and adds an extensive comment to the test explaining what you were asking about.
Attachment #687248 - Attachment is obsolete: true
Attachment #687248 - Flags: review?(gavin.sharp)
Attachment #687248 - Flags: review?(dolske)
Attachment #687963 - Flags: review?(gavin.sharp)
The "aExpectSuccess" bit was confusing me. Since no callers pass true, can you just remove it?
Posted patch Patch (v4)Splinter Review
Oh, right!  Done.  Sorry about that, I should've removed it before.
Attachment #687963 - Attachment is obsolete: true
Attachment #687963 - Flags: review?(gavin.sharp)
Attachment #688086 - Flags: review?(gavin.sharp)
Attachment #688086 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/57be4b233874
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 818459
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.
Whiteboard: [testday-20130104]
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.
Status: RESOLVED → VERIFIED
Depends on: 858916
Depends on: 985777
Depends on: 987119
Depends on: 987125
You need to log in before you can comment on or make changes to this bug.