807217 - Disable Social in private browsing mode

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[(no longer active)]

We need to figure out what the interaction of the social API and private browsing should look like.  Currently what happens is that when you go into private browsing mode, the social sidebar goes back to the login page and when you switch back it comes back up.  Is this the desired behavior?  If yes, perhaps we should close it and reopen it when the user comes back.  (Or perhaps hook it up to the sessionstore if it's not already.)

Is there anything else that we need to handle?  Has this been considered in the privacy review?

Comment 1

[(no longer active)]

We should track this at least for a decision for 17.

Comment 2

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

See the previous bugs about this: bug 804646, bug 774385. I am not aware of any problems with our current behavior.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

This is a sort of meta bug and it seems that if there are known issues (and according to comment 2, this discussion has already occurred in other places) there should be individual, actionable bugs filed. Those issues can be tracked as related to shipping quality/stability/privacy regressions.

For those reasons, not tracking this bug.

Comment 4

[(no longer active)]

OK, so I spent a bunch of time thinking about this, and I believe that the recommend and share functions need to be disabled in private browsing mode, because we have no control over what the other end does with the submitted data, and they seem to be the only places where we share the URLs of visited pages with the providers.  Morphing this bug to that effect.

I also think that this is something that we need to do for 17, so resetting the tracking flags.

As far as the implementation goes, we should use the per-window PB APIs.  Please ping me if you need help with that, and flag me for reivews.

Thanks, and sorry for being late in this game!

Comment 5

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

There are anecdotes that some people use private browsing specifically for interacting with social networks - so wouldn't such people desire these features?

The only way we can interact with social networks in private browsing is if the user explicitly logs into the network while in private browsing (as they are effectively logged out when entering private browsing) so I don't see the risk here?

(As an aside, I believe there is a current risk of some information leakage as we don't actually restart the provider when switching into private browsing mode.  IMO we should do that, but I see no real issue with re-enabling the provider after PB has been entered so a user is free to log in to the network with whatever identity they choose and continue to interact with it)

Comment 6

[(no longer active)]

(In reply to Mark Hammond (:markh) from comment #5)
> There are anecdotes that some people use private browsing specifically for
> interacting with social networks - so wouldn't such people desire these
> features?
> 
> The only way we can interact with social networks in private browsing is if
> the user explicitly logs into the network while in private browsing (as they
> are effectively logged out when entering private browsing) so I don't see
> the risk here?

The thing is that their session should still be considered private.  So, the points at which we exchange URLs of pages being visited with the provider should be considered privacy sensitive.  We're only talking about disabling the browser integration for the share/recommand actions, users are free to use the UI provided by the network for those tasks.

Do you think that is not reasonable?

> (As an aside, I believe there is a current risk of some information leakage
> as we don't actually restart the provider when switching into private
> browsing mode.  IMO we should do that, but I see no real issue with
> re-enabling the provider after PB has been entered so a user is free to log
> in to the network with whatever identity they choose and continue to
> interact with it)

What does restarting really mean?  I'm not entirely sure what currently happens when you go into private browsing mode, but if we can prevent the providers from inferring that the user is now in a private browsing session, maybe we should.

Comment 7

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

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> (In reply to Mark Hammond (:markh) from comment #5)
> The thing is that their session should still be considered private.  So, the
> points at which we exchange URLs of pages being visited with the provider
> should be considered privacy sensitive.  We're only talking about disabling
> the browser integration for the share/recommand actions, users are free to
> use the UI provided by the network for those tasks.
> 
> Do you think that is not reasonable?

I guess it depends :)  If people are treating PB mode more an "incognito" then I see no reason they would not want the ability to also share pages when in that mode.  If people really do want "private" browsing, they aren't likely to log into the social networks in the first place.

I personally don't care at all, I'm just trying to offer a different perspective based on the realization that people use PB mode explicitly for interacting with social networks.

> > (As an aside, I believe there is a current risk of some information leakage
> > as we don't actually restart the provider when switching into private
> > browsing mode.  IMO we should do that, but I see no real issue with
> > re-enabling the provider after PB has been entered so a user is free to log
> > in to the network with whatever identity they choose and continue to
> > interact with it)
> 
> What does restarting really mean?

Restarting means removing all windows (hidden and visible) with their content and re-creating them.

>  I'm not entirely sure what currently
> happens when you go into private browsing mode, 

Not much at all actually.  We currently rely on the social network to do the right thing WRT noticing the cookies have vanished and/or new cookies have taken their place.

I opened bug 807505 on this.

Comment 8

[u279076]

I've always thought of Private Browsing mode to mean "hide my current browsing session for others using the same computer". I still expect to be able to use all the functionality of "normal" browsing mode but to have any local artifacts (cookies, cache, history, etc) disappear when I close my session. Perhaps I'm using it wrong?

Comment 9

[(no longer active)]

(In reply to comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #6)
> > (In reply to Mark Hammond (:markh) from comment #5)
> > The thing is that their session should still be considered private.  So, the
> > points at which we exchange URLs of pages being visited with the provider
> > should be considered privacy sensitive.  We're only talking about disabling
> > the browser integration for the share/recommand actions, users are free to
> > use the UI provided by the network for those tasks.
> > 
> > Do you think that is not reasonable?
> 
> I guess it depends :)  If people are treating PB mode more an "incognito" then
> I see no reason they would not want the ability to also share pages when in
> that mode.  If people really do want "private" browsing, they aren't likely to
> log into the social networks in the first place.

Well, we should treat PB as "private" browsing where we can.

> I personally don't care at all, I'm just trying to offer a different
> perspective based on the realization that people use PB mode explicitly for
> interacting with social networks.

Yep, understood and much appreciated.  Really, it's not 100% clear where to draw these types of lines...

> > > (As an aside, I believe there is a current risk of some information leakage
> > > as we don't actually restart the provider when switching into private
> > > browsing mode.  IMO we should do that, but I see no real issue with
> > > re-enabling the provider after PB has been entered so a user is free to log
> > > in to the network with whatever identity they choose and continue to
> > > interact with it)
> > 
> > What does restarting really mean?
> 
> Restarting means removing all windows (hidden and visible) with their content
> and re-creating them.

I see.  Then we should probably do that as well.

> >  I'm not entirely sure what currently
> > happens when you go into private browsing mode, 
> 
> Not much at all actually.  We currently rely on the social network to do the
> right thing WRT noticing the cookies have vanished and/or new cookies have
> taken their place.
> 
> I opened bug 807505 on this.

Thanks!

Comment 10

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

Per email thread, we're just going to disable the feature entirely in private browsing mode, since that's simplest.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: v1

patch from bug 774385 updated.  This was turned down in bug 774385, but we need a fix for 17 now, future per-window private browsing will have to have a different fix.

Comment 13

[(no longer active)]

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

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

Can you please use PrivateBrowsingUtils.isWindowPrivate instead of the old API?

Thanks!

Comment 14

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

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Can you please use PrivateBrowsingUtils.isWindowPrivate instead of the old
> API?

This is a JS module with no window. I know we need to figure out a per-window solution for trunk, but we're trying to get this into beta 5, so I think this patch will do for now.

Comment 15

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

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

This needs a test!

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > Can you please use PrivateBrowsingUtils.isWindowPrivate instead of the old
> > API?
> 
> This is a JS module with no window. I know we need to figure out a per-window
> solution for trunk, but we're trying to get this into beta 5, so I think this
> patch will do for now.

OK, then please file a follow-up for that.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: v1 + tests

The tests in this patch will fail without the patch in bug 808171.

Comment 18

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=fdc19c9d9061

Comment 19

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

Comment on attachment 677875 [details] [diff] [review]
v1 + tests

>diff --git a/browser/base/content/test/browser_social.js b/browser/base/content/test/browser_social.js

>+  testPrivateBrowsing2: function(next) {

>+    Services.obs.addObserver(function observer(aSubject, aTopic) {
>+      Services.obs.removeObserver(observer, aTopic);
>+      ok(!Social.enabled, "Social is not enabled");
>+      next();
>+      return;
>+      togglePrivateBrowsing(function () {

This doesn't look right...

Otherwise the tests look good too.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #19)
> Comment on attachment 677875 [details] [diff] [review]
> v1 + tests
> 
> >diff --git a/browser/base/content/test/browser_social.js b/browser/base/content/test/browser_social.js
> 
> >+  testPrivateBrowsing2: function(next) {
> 
> >+    Services.obs.addObserver(function observer(aSubject, aTopic) {
> >+      Services.obs.removeObserver(observer, aTopic);
> >+      ok(!Social.enabled, "Social is not enabled");
> >+      next();
> >+      return;
> >+      togglePrivateBrowsing(function () {
> 
> This doesn't look right...

sigh.  yeah, I did that when I was minimizing the test to figure out why it wasn't working.  I'll fix and rerun try.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: v1 + tests

the tests now run and pass, however, I see this in the console:

###!!! ASSERTION: Privacy status of parent docshell doesn't match global state!: 'inPrivateBrowsing == mInPrivateBrowsing', file /Users/shanec/moz/m-c/docshell/base/nsDocShell.cpp, line 2782

Comment 22

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

Ehsan, do you have any idea why the test in that patch would trigger that assertion?

Comment 23

[Shane Caraveo (:mixedpuppy)]

I've added bug 808215 to deal with per-window private browsing later.

Comment 24

[(no longer active)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #22)
> Ehsan, do you have any idea why the test in that patch would trigger that
> assertion?

What is the stack for the assertion?

Comment 25

[Josh Matthews [:jdm]]

Just so you know, that assertion shows up whenever the user leaves global private browsing mode, so it's probably not something to worry about.

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #22)
> > Ehsan, do you have any idea why the test in that patch would trigger that
> > assertion?
> 
> What is the stack for the assertion?

This is the full output in console:

###!!! ASSERTION: Privacy status of parent docshell doesn't match global state!: 'inPrivateBrowsing == mInPrivateBrowsing', file /Users/shanec/moz/m-c/docshell/base/nsDocShell.cpp, line 2782
OBJECT_TO_JSVAL_IMPL [jsval.h:718]
(anonymous namespace)::nsCopyFaviconCallback::~nsCopyFaviconCallback()
xpc::XrayWrapper<js::SecurityWrapper<js::DirectWrapper>, xpc::XPCWrappedNativeXrayTraits>::getOwnPropertyDescriptor(JSContext*, JSObject*, jsid, bool, JSPropertyDescriptor*) [js/xpconnect/wrappers/XrayWrapper.cpp:1467]
non-virtual thunk to xpc::XrayWrapper<js::SecurityWrapper<js::DirectWrapper>, xpc::XPCWrappedNativeXrayTraits>::defineProperty(JSContext*, JSObject*, jsid, JSPropertyDescriptor*)
nsDocumentFragment::GetNamespaceURI(nsAString_internal&) [content/base/src/nsDocumentFragment.cpp:34]
nsCOMPtr<nsIDocumentEncoderNodeFixup>::nsCOMPtr(nsQueryInterface)
nsHTMLCopyEncoder::IsLastNode(nsIDOMNode*) [content/base/src/nsDocumentEncoder.cpp:1973]
nsHTMLCopyEncoder::IsFirstNode(nsIDOMNode*) [content/base/src/nsDocumentEncoder.cpp:1934]
nsHTMLCopyEncoder::IsFirstNode(nsIDOMNode*) [content/base/src/nsDocumentEncoder.cpp:1902]
nsGridRowLeafFrame::GetBorderAndPadding(nsMargin&) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:89]
nsGridRowLeafFrame::GetBorderAndPadding(nsMargin&) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:50]
nsGenericDOMDataNode::SplitData(unsigned int, nsIContent**, bool) [content/base/src/nsGenericDOMDataNode.cpp:719]
NS_NewGridRowLeafFrame(nsIPresShell*, nsStyleContext*) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:31]
nsGenericDOMDataNode::SplitData(unsigned int, nsIContent**, bool) [content/base/src/nsGenericDOMDataNode.cpp:719]
NS_NewGridRowLeafFrame(nsIPresShell*, nsStyleContext*) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:31]
nsGenericDOMDataNode::SplitData(unsigned int, nsIContent**, bool) [content/base/src/nsGenericDOMDataNode.cpp:719]
NS_NewGridRowLeafFrame(nsIPresShell*, nsStyleContext*) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:31]
nsGenericDOMDataNode::SplitData(unsigned int, nsIContent**, bool) [content/base/src/nsGenericDOMDataNode.cpp:719]
NS_NewGridRowLeafFrame(nsIPresShell*, nsStyleContext*) [layout/xul/base/src/grid/nsGridRowLeafFrame.cpp:31]
nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsIDocument*, unsigned int) [content/base/src/nsImageLoadingContent.cpp:664]
nsCOMPtr<nsIDOMSerializer>::assign_from_helper(nsCOMPtr_helper const&, nsID const&) [nsCOMPtr.h:1204]
nsImageLoadingContent::AutoStateChanger::~AutoStateChanger() [content/base/src/nsImageLoadingContent.h:199]
nsTextNode::List(__sFILE*, int) const [content/base/src/nsTextNode.cpp:173]
nsGenericHTMLElement::GetURIAttr(nsIAtom*, nsIAtom*, nsAString_internal&) [content/html/content/src/nsGenericHTMLElement.cpp:2939]
xpc_qsXPCOMObjectToJsval(XPCLazyCallContext&, qsObjectHelper&, nsID const*, XPCNativeInterface**, JS::Value*) [js/xpconnect/src/XPCQuickStubs.cpp:985]
js::AddOperation [js/src/jsinterpinlines.h:524]
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2681]
js::ReportIsNotFunction(JSContext*, JS::Value const*, js::MaybeConstruct) [js/src/jsinterp.cpp:246]
TypeConstraintSetElement::TypeConstraintSetElement(JSScript*, unsigned char*, js::types::StackTypeSet*, js::types::StackTypeSet*)
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2695]
js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) [js/src/jsobj.cpp:4942]
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2780]
js::Vector<DecompiledOpcode, 0ul, js::TempAllocPolicy>::~Vector() [./../../dist/include/js/Vector.h:554]
js::WeakMap<js::EncapsulatedPtr<JSObject, unsigned long>, js::RelocatableValue, js::DefaultHasher<js::EncapsulatedPtr<JSObject, unsigned long> > >::markIteratively(JSTracer*) [js/src/jsweakmap.h:169]
JSObject::addDataProperty(JSContext*, jsid, unsigned int, unsigned int) [js/src/jsobj.h:780]
IdToValue [js/src/jsproxy.cpp:1303]
ScriptedDirectProxyHandler::keys(JSContext*, JSObject*, JS::AutoIdVector&) [js/src/jsproxy.cpp:2097]
js::AddOperation [js/src/jsinterpinlines.h:524]
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2666]
js::ReportIsNotFunction(JSContext*, JS::Value const*, js::MaybeConstruct) [js/src/jsinterp.cpp:246]
TypeConstraintSetElement::TypeConstraintSetElement(JSScript*, unsigned char*, js::types::StackTypeSet*, js::types::StackTypeSet*)
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2695]
js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) [js/src/jsobj.cpp:4942]
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2780]
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [js/src/jsinterp.cpp:2869]
js_DumpStackFrame(JSContext*, js::StackFrame*) [js/src/jsobj.cpp:5493]
js_InitClass(JSContext*, JS::Handle<JSObject*>, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, JS::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*, JSObject**, js::gc::AllocKind) [js/src/jsobj.cpp:3222]
js::types::HashSetLookup<jsid, js::types::Property, js::types::Property> [js/src/jsinferinlines.h:1233]
JS_DefinePropertyById [js/src/jsapi.cpp:3832]
DefinePropertyById [js/src/jsapi.cpp:3791]
nsXPCWrappedJS::UnmarkIfPurple()
NativeSetMap::Entry::Match(JSDHashTable*, JSDHashEntryHdr const*, void const*) [js/xpconnect/src/XPCMaps.cpp:403]
MemoryReporter_PageFaultsSoft::GetKind(int*) [xpcom/base/nsMemoryReporterManager.cpp:382]

Comment 27

[Shane Caraveo (:mixedpuppy)]

I'll ignore the assertion based on comment 25, new try push at

https://tbpl.mozilla.org/?tree=Try&rev=b0e8cda8eb82

Comment 28

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

Comment on attachment 677905 [details] [diff] [review]
v1 + tests

>diff --git a/browser/base/content/test/browser_social.js b/browser/base/content/test/browser_social.js

>+          // observe again and wait for the enable to occur
>+          Services.obs.addObserver(function endobserver(aSubject, aTopic) {
>+            Services.obs.removeObserver(endobserver, aTopic);
>+            next();
>+          }, "social:pref-changed", false);
>+          Social.enabled = enabledAtStart;

I'm not sure why this (or enabledAtStart) is needed. During the test, isn't enabledAtStart guaranteed to be true? And this test seems to be relying on that, since if it's false then there will not be any notification and the test will timeout. 

>diff --git a/browser/modules/Social.jsm b/browser/modules/Social.jsm

>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic == "private-browsing") {
>+      if (aData == "enter") {
>+        this._enabledBeforePrivateBrowsing = this.enabled;
>+        if (this.enabled)
>+          this.enabled = false;
>+      } else if (aData == "exit") {
>+        if (this.enabled != this._enabledBeforePrivateBrowsing)
>+          this.enabled = this._enabledBeforePrivateBrowsing;
>+      }

You should be able to continue unconditionally setting these, right? If it's necessary to avoid unnecessary notifications, we should fix the SocialService setter to not notify when setting to the current state.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)

Removing checkin-needed pending these comments.

Comment 30

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

Comment on attachment 677905 [details] [diff] [review]
v1 + tests

Shane, can we get an updated patch here?

Comment 31

[Shane Caraveo (:mixedpuppy)]

Attachment: v2 with tests

waiting on try server https://tbpl.mozilla.org/?tree=Try&rev=88f4e60f15fd

Comment 32

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

Comment on attachment 678815 [details] [diff] [review]
v2 with tests

>diff --git a/browser/base/content/test/browser_social.js b/browser/base/content/test/browser_social.js

>+// a place for miscelaneous social tests

miscellaneous

>+  testPrivateBrowsing: function(next) {

>+          // since we're expecting pb toggling to result in a pref-changed
>+          // notification, we watch for that to happen rather than the
>+          // pb transition notifications.
>+          togglePrivateBrowsing(function () {
>+            ok(!Social.enabled, "Social shuts down during private browsing");
>+            togglePrivateBrowsing(function () {
>+              ok(Social.enabled, "Social enabled after private browsing");

Doing the test this way isn't ideal - if the disable-in-PB code is broken, this will time out rather than failing the check (since you're observing the "pref-changed" notification rather than the private browsing notification). Why can't you do this from the privatebrowsing-transition notification?

>+function togglePrivateBrowsing(aCallback, topic) {
>+  // we could watch for private-browsing-transition-complete, but we really
>+  // want to know when "social:pref-changed" is fired

This comment seems misplaced (but hopefully you can get rid of it if you address the comment above).

>diff --git a/browser/base/content/test/head.js b/browser/base/content/test/head.js

I don't understand these changes - I don't think they're necessary, so I'd like to omit them.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: v2 with tests

short try at https://tbpl.mozilla.org/?tree=Try&rev=89634f05ca95 to see if we get past previous try errors

Comment 34

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

Comment on attachment 678828 [details] [diff] [review]
v2 with tests

r=me assuming the tests pass and such! Can you remove the "The sidebar message will always come first" comment? That was copy/pasted into areas where it no longer actually applies...

Comment 35

[Shane Caraveo (:mixedpuppy)]

Attachment: v2 with tests

removed comments per review, no other changing, keeping r+ from gavin.

assuming last try will work, pushed full try at

https://tbpl.mozilla.org/?tree=Try&rev=5749eb47b36f

Comment 36

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

Attachment: Version without the _initialized but a simple flag just for the observer

Comment 37

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdc4d86fc39

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 678960 [details] [diff] [review]
Version without the _initialized but a simple flag just for the observer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Social
User impact if declined: social won't be disabled on private browsing
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): hopefully small, but i'm not sure
String or UUID changes made by this patch: none

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 678960 [details] [diff] [review]
Version without the _initialized but a simple flag just for the observer

We have to take this and just do as much testing around it as possible in the coming week before Beta 6 so approving branch uplift as soon as the builds on inbound show positive results.

Comment 40

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/975a3df77b9b

Comment 41

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d0a3931168de

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cdc4d86fc39

Comment 43

[Simona Badau, Desktop QA]

I was doing some exploratory testing around Social API and Private Browsing and I want to make sure that what I see is the right behavior. 

Disabling Social API per default in Private Browsing does not mean that the feature can't be enabled if the user wants to (by setting the pref social.enabled to true while in PB), right?

If the Social API is manually enabled while in Private Browsing mode should the user be able to use it normally?

What happens now is that the user can enable Social API, the sidebar is available and all it's content is usable except for the fact that when trying to chat with someone the message "You are not online and cannot send any messages" is displayed. The confusion here can appear because, what's the point in being able to enable it but not use it.

Comment 44

[Simona Badau, Desktop QA]

I should mention that this is an intermittent behavior and does not reproduce all the time. When the message "You are not online and cannot send any messages" does not appear the Social API can be used for chatting.

Comment 45

[Ioana (away)]

On Firefox 17 beta 5 (20121106195758) the following issue happened:
1. Go to https://www.facebook.com/about/messenger-for-firefox and enable SocialAPI.
2. Log into an account.
3. After your friends and notifications are listed in the sidebar, start Private Browsing.
4. Repeat steps 1 and 2 while in Private Browsing mode. (This issue reproduced both when using the same FB account, and when using a different FB account.)
5. Exit PB mode.

The user stays logged into his Facebook account but the sidebar looses all his account information - it goes back to the Login button sidebar. It is enough to restart the browser to get the information back on the sidebar, but this is a quite poor user experience.

Comment 46

[(no longer active)]

Hmm, shouldn't this patch have disabled social in PB mode completely?

Comment 47

[u279076]

Here is what I am seeing with 17.0b5 on Windows 8:

1. Go to https://www.facebook.com/about/messenger-for-firefox
2. Log in to Facebook
3. Click the green "Turn On" button
> Social API is activated with activity feed, friends list, and toolbar
4. Start Private Browsing mode
> Social API is deactivated
5. Repeat steps 1-3
> Social API is activated
6. Exit private browsing mode
> Social API is restored (feed, friends, and toolbar still active)

Given that the goal here was to have Social API non-existent in Private Browsing mode I think we need to reopen this bug.

Comment 48

[Shane Caraveo (:mixedpuppy)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #47)
> Given that the goal here was to have Social API non-existent in Private
> Browsing mode I think we need to reopen this bug.

The goal was to disable social when entering private browsing so that we ensured the social provider is clean.  I don't see any reason to prevent the user from re-enabling social while in pb mode (though that will change with per-window).  

This does bring up a bug though that should be fixed.  Then exiting pb mode, we restore the enabled state they had when entering pb mode.

enabled->enter pb->disabled->exit pb->enabled

disabled->enter pb->disabled->exit pb->disabled

However, if the user was enabled, enters pb and re-enables, upon exiting pb mode the provider is not being flushed properly.

So, we should either fix that or actually prevent enabling during pb.  Both are simple patches, we should do one or the other for release.  I don't feel like this should block beta 5.  I am reopening this for now.

Comment 49

[u279076]

Thanks Shane, I agree with that approach and that waiting for these changes should not block Beta 5.

Comment 50

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

Let's track comment 43-comment 49 in a followup bug - the weirdness around enabling social in private browsing is an issue, but I don't think it's a blocking one, and tracking multiple landings in the same bug really doesn't work very well :)

Comment 51

[(no longer active)]

Please CC me on the follow-up bug.  Thanks!

Comment 52

[u279076]

Verified this is working as expected (apart from the follow-up issues) in Firefox 17b5, today's 18.0a2 Aurora, and today's 19.0a1 Nightly. Marking this verified.