Closed Bug 580158 Opened 11 years ago Closed 11 years ago

Change Firefox Sync behavior in Private Browsing mode


(Firefox :: Sync, defect)

Not set





(Reporter: rags, Assigned: philikon)



(Whiteboard: [fixed-1.4.2])


(1 file, 1 obsolete file)

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.
Targeting 1.4.2.
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.
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.
Depends on: 481345
Attached patch v1 (obsolete) — Splinter Review
The patch doesn't do anything actively to ignore history as private browsing will prevent history events from triggering (because history isn't saved).
Attachment #458568 - Flags: review?(mconnor)
Comment on attachment 458568 [details] [diff] [review]

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)
Attachment #458568 - Flags: review?(mconnor) → review+
Attached patch v2Splinter Review
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.
Assignee: nobody → philipp
Attachment #458568 - Attachment is obsolete: true
Attachment #458822 - Flags: review?(mconnor)
Attachment #458822 - Flags: review?(mconnor) → review+
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.
Closed: 11 years ago
Flags: blocking-fx-sync1.5+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [fixed-1.4.2]
Target Milestone: --- → 1.5
Blocks: 580704
Verified fix on 1.4.2b1
Duplicate of this bug: 579525
Flagging in-testsuite for inclusion of tests in crossweave
Flags: in-testsuite?
Tests to check form data behavior in private browsing mode -

Tests to check passwords behavior in private browsing mode -

Tests to check tabs behavior in private browsing mode -
Flags: in-testsuite? → in-testsuite+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.