580158 - Change Firefox Sync behavior in Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ragavan S [:rags]]

Currently, Firefox Sync disables itself if:

1. The user has enabled Private Browsing mode.
2. The user has set "Never Remember History" on.

This behavior is not ideal and causes unexpected behavior as was seen with the Firefox Home users who set up their account but didn't upload any data to the servers.

We plan to change the behavior of Firefox Sync to do the following:

1. If the user has enabled Private Browsing mode:

Initial setup will be allowed and will upload data from all the engines selected by the user. By default, this will mean all data engines. Note that this is data that was persisted locally by Firefox either outside of the Private Browsing session or by an explicit user request to persist such data while in Private Browsing session. Upon completion of setup, the user will continue to remain in Private Browsing mode.

Once setup, Firefox Sync will continue running even when the user is in Private Browsing mode. No data will actually be synced except data that is locally persisted in response to an explicit user request. For example, users can choose to remember passwords or store bookmarks while in Private Browsing mode. Such data will be synced. No history or tabs will be synced while the user is in Private Browsing mode.

2. If the user has set "Never Remember History" on.

Initial setup will be allowed and will upload data from all the engines selected by the user. This should default to all engines, unless there is no history in the user's profile in which case we should exclude the history engine. On an ongoing basis, Firefox Sync will continue syncing all the engines enabled during setup except History.

Comment 1

[Ragavan S [:rags]]

Targeting 1.4.2.

Comment 2

[Ed Lee :Mardak]

We probably don't need to be so heavy-weight with the private browsing syncing. History isn't generated during private browsing. Bookmarks are persisted when moving out of private browsing.

We should only really be checking if private browsing is enabled from tabs engine and not sync *out* any data. It can still download incoming tabs.

Comment 3

[Ed Lee :Mardak]

So instead of checking if the user is in private browsing and stopping sync, just make sure the tab tracker is empty and doesn't track new stuff during private browsing.

Even the original bug 481345 is only concerned about uploading tabs.

Comment 4

[Ed Lee :Mardak]

Attachment: v1

The patch doesn't do anything actively to ignore history as private browsing will prevent history events from triggering (because history isn't saved).

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 458568 [details] [diff] [review]
v1

this goes further than necessary, as it will disable tab sync in "never remember history" mode.  The suggestion yesterday was to only disable tab sync when the user was in "Private Browsing" mode.

>+    // Don't provide any tabs to compare against and ignore the update later
>+    if (Svc.Private.privateBrowsingEnabled) {

    if (Svc.Private.privateBrowsingEnabled && !Svc.Private.autoStarted) {

>   getAllIDs: function TabStore_getAllIds() {
>+    // Don't report any tabs if we're in private browsing for first syncs
>     let ids = {};
>+    if (Svc.Private.privateBrowsingEnabled)

same as above


>@@ -240,19 +256,26 @@ TabTracker.prototype = {
>     if (aTopic == "domwindowopened") {
>       let self = this;
>       aSubject.addEventListener("load", function onLoad(event) {
>         aSubject.removeEventListener("load", onLoad, false);
>         // Only register after the window is done loading to avoid unloads
>         self._registerListenersForWindow(aSubject);
>       }, false);
>     }
>+    else if (aTopic == "private-browsing")
>+      this.clearChangedIDs();

edgecase alert.  If a user disables "never remember history" this notification will be sent, and we won't sync tabs on the next sync even though they should.  We only need to clear going into PB, as when we're exiting, we do expect/want to sync tabs on the next sync. (and the tabs are retrieved at sync time, so this should be okay).

>   onTab: function onTab(event) {
>+    if (Svc.Private.privateBrowsingEnabled) {

same check as above.

need a test for this (open a tab, verify we have changes to sync, enter PB, verify none, open tabs, verify none, exit PB, verify none, open tabs, verify some)

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Differentiate between explicit Private Browsing mode (entered via menu item or -private flag) and "Never remember history". In the former case we don't have the tabs engine provide data. In the latter we just function normally. This addresses the review comments for v1.

Comment 7

[Ed Lee :Mardak]

http://hg.mozilla.org/services/fx-sync/rev/d6d1d2eb266c
Don't disable sync when in private browsing. When in explicit private browsing mode (either via menu item or -private command line flag), have tabs engine not provide data. 

http://hg.mozilla.org/services/fx-sync/rev/21c80eb01d6b
1.4.x

Comment 8

[Tony Chung [:tchung]]

Verified fix on 1.4.2b1

Comment 10

[Ayan Shah[:ashah]]

Flagging in-testsuite for inclusion of tests in crossweave

Comment 11

[Ayan Shah[:ashah]]

Tests to check form data behavior in private browsing mode - 
http://hg.mozilla.org/automation/crossweave/file/4843f30a04c7/tests/private_browsing_formdata.test

Tests to check passwords behavior in private browsing mode - 
http://hg.mozilla.org/automation/crossweave/file/4843f30a04c7/tests/private_browsing_passwords.test

Tests to check tabs behavior in private browsing mode - 
http://hg.mozilla.org/automation/crossweave/file/4843f30a04c7/tests/private_browsing_tabs.test