844671 - Tabs sent through Sync should not open in private browsing windows

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Tom Lowenthal]

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.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Only sync remote tabs to non-private windows.

Comment 2

[Richard Newman [:rnewman]]

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?

Comment 3

[Josh Matthews [:jdm]]

Attachment: Only sync remote tabs to non-private windows.

Thanks Richard. Awkward.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Only sync remote tabs to non-private windows.

Sorry, qref.

Comment 5

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

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?

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[(no longer active)]

(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!  ;-)

Comment 8

[Richard Newman [:rnewman]]

(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.

Comment 9

[(no longer active)]

(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.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Only sync remote tabs to non-private windows.

Comment 11

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

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').

Comment 12

[Josh Matthews [:jdm]]

Filed bug 846911 for the audit.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f18d05d1ba1

Comment 14

[Josh Matthews [:jdm]]

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

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c38206da16d5
https://hg.mozilla.org/releases/mozilla-beta/rev/e56bc3efb690

Comment 16

[Virgil Dicu [:virgil] [QA]]

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

Comment 17

[u279076]

Flagging for verification in Firefox 21 and 22.

Comment 18

[Virgil Dicu [:virgil] [QA]]

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.

Comment 19

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old