809997 - exiting private browsing may leave provider running

Status: RESOLVED DUPLICATE of bug 815042

(Firefox Graveyard :: SocialAPI, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

Bug 807217 disables social when entering pb, and restores when exiting if necessary.  However, if the user enables social while in pb and then exits, the provider is left in an unknown state since cookies have been pulled from underneath them.

See bug 807217 comment 43 through 49 for some details.

We can fix this one of two ways:

- prevent enabling social during pb
- if social is enabled during pb, reload the provider on exiting pb

Since it is likely we will have to prevent social during pb when per-window pb lands, I will write up a patch for the first case.

Comment 1

[(no longer active)]

I don't know what it means for the provider to not be reloaded.  Can you please explain what the bad behavior is?  Thanks!

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent social during pb.patch

This prevents enabling social during pb

Comment 3

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

Having the menu item be enabled but fail to work isn't a great experience - we should hide it in PB mode if we're going to do this.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent social during pb.patch

this also sets active to false, hiding any menu's

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 679862 [details] [diff] [review]
prevent social during pb.patch

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

This patch is on the right path, but I would prefer that we still allow people to use the Social API while in PB mode. There is a definite use-case of users who want to segregate their social browsing from non-social browsing, and I don't want us to get rid of that if we don't have to.

Also, on IRC there was talk about moving some of this logic to SocialService so that it stands up to about:config toggles. I'd like to see how much different that makes this code.

::: browser/base/content/test/browser_social.js
@@ +43,5 @@
> +               "Social menus hidden during private browsing")
> +            // try to enable social
> +            Social.enabled = true;
> +            ok(!Social.enabled, "Cannot reenable social during private browsing");
> +            

nit: leading whitespace

::: browser/modules/Social.jsm
@@ +61,1 @@
>          this.enabled = this._enabledBeforePrivateBrowsing;

I would prefer that we just fix the Social.active setter to not also set the enabled state. There are three places where the Social.active setter is used, so adding an extra line to those places to also set Social.enabled wouldn't be a pain and it would make it more obvious what is happening. It would also make this part of the code far less confusing.

Whether we make this change on 17 or only on m-c is really the question here. I don't want to introduce some other regression by doing too much refactoring, so I'd say to leave it as-is on 17 and 18, but then on 19 make this refactoring.

Also, this comment can be better worded. How about:
> // The |Social.active| setter cannot be used because it sets |Social.enabled|
> // to the same value as |Social.active|, which is not always true in this case.

Comment 6

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

Comment on attachment 679862 [details] [diff] [review]
prevent social during pb.patch

I agree with Jared - I don't think we should just disable this entirely, since that complicates too many other interactions. The simple fix here would be to always cause a state transition across private browsing transitions, even if we're going to be restoring the service to its original state.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent social during pb.patch

This fixes preventing the enable from pref toggle as discussed in irc, in case we choose to go this route.  another patch coming.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: reload provider on pb exit

this alternative reloads the provider if necessary during pb exit.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680271 [details] [diff] [review]
prevent social during pb.patch

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

Clearing review request since we want to keep allowing users to use the Social API while in pb mode.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680272 [details] [diff] [review]
reload provider on pb exit

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

Please try to see if the suggested code will work since I *hope* that it will be more dependable and easy to follow.

::: browser/base/content/test/browser_social.js
@@ +79,5 @@
>          });
>        });
>      }, "social:pref-changed", false);
> +  },
> +  

nit: leading whitespace

@@ +112,5 @@
> +                  case "got-sidebar-message":
> +                    ok(true, "Social working during private browsing");
> +                    port.close();
> +
> +                    // not sure how to best get the right async order in these next two steps

Yeah, this part is a little confusing, because the pref changes twice here, but only the first change is getting noticed. The second one is picked up by the togglePrivateBrowsing callback.

What if you change it to:
observePrefChanged(function prefShouldBeDisabledUponExitingPbMode() {
  ok(!Social.enabled, "Social disabled notification received");
  ok(!toggled, "have not yet received pb exit (good thing)");
  observePrefChanged(function prefShouldBeReenabledUponEnteringNonPbMode() {
    is(Social.enabled, "Social re-enabled after private browsing");
    toggled = true;
    next();
  });
});
togglePrivateBrowsing();

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Oops - sorry, I didn't see this bug before, but bug 815042 should have resolved this - closing this one as the other has landed a patch.

Comment 12

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

Comment on attachment 680272 [details] [diff] [review]
reload provider on pb exit

Sorry that I dropped the ball on this one :(