Closed Bug 844671 Opened 7 years ago Closed 7 years ago

Tabs sent through Sync should not open in private browsing windows

Categories

(Firefox :: Private Browsing, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox19 --- unaffected
firefox20 + verified
firefox21 + verified
firefox22 --- fixed

People

(Reporter: me, Assigned: jdm)

References

Details

Attachments

(1 file, 3 obsolete files)

If I have a private browsing window in focus when I leave my main computer alone and later send some tabs from a mobile device to my computer using Firefox Sync, those tabs will appear in the private window, not one of the normal windows. Instead, those tabs should appear only non-private windows.
Component: General → Private Browsing
Blocks: PBnGen
Attachment #717738 - Flags: review?(rnewman)
Assignee: nobody → josh
Comment on attachment 717738 [details] [diff] [review]
Only sync remote tabs to non-private windows.

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

*Obi-Wan* This is not the code you're looking for. 

You're looking for http://dxr.mozilla.org/mozilla-central/browser/components/nsBrowserGlue.js#l1586 .

TPS is a test runner for Sync; all you're doing here is ensuring that TPS tests never open tabs in private windows, which probably breaks TPS's private window tests!

::: services/sync/tps/extensions/tps/modules/tabs.jsm
@@ +11,5 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://services-sync/main.js");
> +Cu.import("resource:///modules/RecentWindow.jsm");

Is that supposed to have a triple-slash?
Attachment #717738 - Flags: review?(rnewman) → review-
Thanks Richard. Awkward.
Attachment #717861 - Flags: review?(gavin.sharp)
Attachment #717738 - Attachment is obsolete: true
Sorry, qref.
Attachment #717863 - Flags: review?(gavin.sharp)
Attachment #717861 - Attachment is obsolete: true
Attachment #717861 - Flags: review?(gavin.sharp)
Comment on attachment 717863 [details] [diff] [review]
Only sync remote tabs to non-private windows.

getMostRecentBrowserWindow is an implementation of nsIBrowserGlue::getMostRecentBrowserWindow, so if you're changing its signature you should probably also update the IDL definition. You can probably just add an optional jsval argument?

I'm not entirely sure why we think this is the correct behavior, though. Why is opening a remote tab in the (focused) private window a problem?
Attachment #717863 - Flags: review?(gavin.sharp) → feedback+
Gavin: I think the ideal is that just because a sync occurs while a user has a PB window open, it doesn't mean any arriving tabs from that sync should be private (and thus not appear in history). Non-private seems a sane default.

Ideally, tabs would retain their private state: a tab sent from PB mode on Android would appear that way on desktop.
(In reply to Richard Newman [:rnewman] from comment #6)
> Gavin: I think the ideal is that just because a sync occurs while a user has
> a PB window open, it doesn't mean any arriving tabs from that sync should be
> private (and thus not appear in history). Non-private seems a sane default.
> 
> Ideally, tabs would retain their private state: a tab sent from PB mode on
> Android would appear that way on desktop.

Private tabs should never be synced in the first place.  Just imagine somebody who was shopping for an engagement ring on their way to work to get there, open their laptop and have the engagement ring tabs open up!  ;-)
(In reply to :Ehsan Akhgari from comment #7)

> Private tabs should never be synced in the first place.

We don't sync private tabs. Send Tab != tab sync.
(In reply to comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> 
> > Private tabs should never be synced in the first place.
> 
> We don't sync private tabs. Send Tab != tab sync.

Oh, I stand corrected then.
Attachment #719945 - Flags: review?(gavin.sharp)
Attachment #717863 - Attachment is obsolete: true
Comment on attachment 719945 [details] [diff] [review]
Only sync remote tabs to non-private windows.

Seems like maybe we need to audit all the non-RecentWindow getMostRecentBrowserWindow callers? Maybe also getMostRecentWindow('navigator:browser').
Attachment #719945 - Flags: review?(gavin.sharp) → review+
Filed bug 846911 for the audit.
https://hg.mozilla.org/mozilla-central/rev/9f18d05d1ba1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment on attachment 719945 [details] [diff] [review]
Only sync remote tabs to non-private windows.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Potential for shared tabs between mobile/desktop appearing in the wrong window.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Attachment #719945 - Flags: approval-mozilla-beta?
Attachment #719945 - Flags: approval-mozilla-aurora?
Attachment #719945 - Flags: approval-mozilla-beta?
Attachment #719945 - Flags: approval-mozilla-beta+
Attachment #719945 - Flags: approval-mozilla-aurora?
Attachment #719945 - Flags: approval-mozilla-aurora+
Verified the issue on 2013-02-27 Nightly (20130227030925). 

I sent a tab from my mobile device to the desktop using Firefox Sync with a Private Window opened. I clicked "Sync now", the tab appeared in the PB window and not in the Normal Window, as expected.

Verified the fix for Firefox 20.0 Beta 3 (20130305164032). Sync tab sent from mobile device is opened in the Normal Window, as expected.

Verified for Windows 7 and Mac OS X 10.8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Flagging for verification in Firefox 21 and 22.
Keywords: verifyme
Verified fix for Firefox 20.0 beta 1 on Windows 7 x64
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Build ID: 20130401192816

Sync tab sent from mobile device is opened in the Normal Window, as expected.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.