Last Comment Bug 807217 - Disable Social in private browsing mode
: Disable Social in private browsing mode
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Shane Caraveo (:mixedpuppy)
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: Shane Caraveo (:mixedpuppy)
Mentors:
: 774385 (view as bug list)
Depends on: 1231465 808171
Blocks: 807505
  Show dependency treegraph
 
Reported: 2012-10-30 21:07 PDT by :Ehsan Akhgari
Modified: 2015-12-09 10:36 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified


Attachments
v1 (1.33 KB, patch)
2012-11-01 17:53 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
Details | Diff | Splinter Review
v1 + tests (5.58 KB, patch)
2012-11-02 13:50 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
v1 + tests (5.94 KB, patch)
2012-11-02 15:17 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
v2 with tests (9.59 KB, patch)
2012-11-06 11:11 PST, Shane Caraveo (:mixedpuppy)
gavin.sharp: review-
Details | Diff | Splinter Review
v2 with tests (5.87 KB, patch)
2012-11-06 11:53 PST, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
Details | Diff | Splinter Review
v2 with tests (5.71 KB, text/plain)
2012-11-06 13:17 PST, Shane Caraveo (:mixedpuppy)
mixedpuppy: review+
Details
Version without the _initialized but a simple flag just for the observer (5.88 KB, patch)
2012-11-06 15:59 PST, Mark Hammond [:markh]
felipc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-10-30 21:07:28 PDT
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 :Ehsan Akhgari 2012-10-30 21:08:19 PDT
We should track this at least for a decision for 17.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 12:17:49 PDT
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 2012-10-31 13:56:44 PDT
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 :Ehsan Akhgari 2012-10-31 15:06:25 PDT
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] 2012-10-31 15:38:01 PDT
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 :Ehsan Akhgari 2012-10-31 15:47:27 PDT
(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] 2012-10-31 15:55:36 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-31 15:59:02 PDT
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 :Ehsan Akhgari 2012-10-31 16:07:57 PDT
(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: gavin@gavinsharp.com] 2012-11-01 14:14:44 PDT
Per email thread, we're just going to disable the feature entirely in private browsing mode, since that's simplest.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-01 14:16:07 PDT
*** Bug 774385 has been marked as a duplicate of this bug. ***
Comment 12 Shane Caraveo (:mixedpuppy) 2012-11-01 17:53:27 PDT
Created attachment 677622 [details] [diff] [review]
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 :Ehsan Akhgari 2012-11-01 18:18:18 PDT
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: gavin@gavinsharp.com] 2012-11-01 19:49:26 PDT
(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: gavin@gavinsharp.com] 2012-11-02 09:47:41 PDT
Comment on attachment 677622 [details] [diff] [review]
v1

This needs a test!
Comment 16 :Ehsan Akhgari 2012-11-02 10:02:30 PDT
(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) 2012-11-02 13:50:12 PDT
Created attachment 677875 [details] [diff] [review]
v1 + tests

The tests in this patch will fail without the patch in bug 808171.
Comment 18 Shane Caraveo (:mixedpuppy) 2012-11-02 13:50:59 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fdc19c9d9061
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-02 14:14:11 PDT
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) 2012-11-02 14:17:57 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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) 2012-11-02 15:17:33 PDT
Created attachment 677905 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-11-02 15:29:30 PDT
Ehsan, do you have any idea why the test in that patch would trigger that assertion?
Comment 23 Shane Caraveo (:mixedpuppy) 2012-11-02 15:39:31 PDT
I've added bug 808215 to deal with per-window private browsing later.
Comment 24 :Ehsan Akhgari 2012-11-02 15:43:46 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] (on vacation until Dec 5) 2012-11-02 15:48:10 PDT
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) 2012-11-02 15:54:46 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com 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) 2012-11-02 16:15:33 PDT
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: gavin@gavinsharp.com] 2012-11-02 16:18:21 PDT
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] 2012-11-03 06:19:36 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28)

Removing checkin-needed pending these comments.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-06 10:22:44 PST
Comment on attachment 677905 [details] [diff] [review]
v1 + tests

Shane, can we get an updated patch here?
Comment 31 Shane Caraveo (:mixedpuppy) 2012-11-06 11:11:55 PST
Created attachment 678815 [details] [diff] [review]
v2 with tests

waiting on try server https://tbpl.mozilla.org/?tree=Try&rev=88f4e60f15fd
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-06 11:26:00 PST
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) 2012-11-06 11:53:25 PST
Created attachment 678828 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-11-06 12:34:49 PST
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) 2012-11-06 13:17:05 PST
Created attachment 678861 [details]
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] 2012-11-06 15:59:17 PST
Created attachment 678960 [details] [diff] [review]
Version without the _initialized but a simple flag just for the observer
Comment 38 :Felipe Gomes (needinfo me!) 2012-11-06 16:54:42 PST
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 2012-11-06 16:58:11 PST
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 me!) 2012-11-06 18:55:32 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/975a3df77b9b
Comment 41 :Felipe Gomes (needinfo me!) 2012-11-06 19:33:51 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0a3931168de
Comment 42 Ed Morley [:emorley] 2012-11-07 04:59:43 PST
https://hg.mozilla.org/mozilla-central/rev/8cdc4d86fc39
Comment 43 Simona B [:simonab ] 2012-11-08 06:33:55 PST
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 B [:simonab ] 2012-11-08 06:51:18 PST
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) 2012-11-08 07:34:01 PST
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 :Ehsan Akhgari 2012-11-08 09:12:28 PST
Hmm, shouldn't this patch have disabled social in PB mode completely?
Comment 47 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-08 10:06:43 PST
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) 2012-11-08 10:18:32 PST
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-08 10:23:13 PST
Thanks Shane, I agree with that approach and that waiting for these changes should not block Beta 5.
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-08 10:36:03 PST
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 :Ehsan Akhgari 2012-11-08 12:35:52 PST
Please CC me on the follow-up bug.  Thanks!
Comment 52 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-08 13:36:32 PST
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.

Note You need to log in before you can comment on or make changes to this bug.