leaving private browsing with social enabled doesn't reset all social components

VERIFIED FIXED in Firefox 17



5 years ago
5 years ago


(Reporter: markh, Assigned: markh)


17 Branch
Firefox 20

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox20+ verified, firefox-esr17+ verified)



(1 attachment, 1 obsolete attachment)



5 years ago
Created attachment 685036 [details] [diff] [review]
Reset all social components as PB exits.

Enter PB mode, then enable social, then leave PB mode.  The social components are not torn down meaning information could leak between the modes.  For example, the sidebar could store the value of document.cookies in a window variable while in PB mode, then after leaving PB mode the variable will still exist.

This snuck through the cracks for a couple of reasons: bug 807505 was initially opened for this but was marked as invalid as bug 807217 initially was intended to prevent social being used in PB mode at all, but then morphed into disabling it but allowing it to be re-enabled.
Attachment #685036 - Flags: review?(gavin.sharp)
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → +
tracking-firefox-esr17: ? → 18+
Will leave this nominated for esr17 - unsure if we would do a chemspill on ESR for this, but we'll track this for a possible 17.0.1.
tracking-firefox-esr17: 18+ → ?
Assignee: nobody → mhammond
Comment on attachment 685036 [details] [diff] [review]
Reset all social components as PB exits.

Good catch, unfortunate that we missed this. :(

A waitForPortMessage(port, "sometopic", callback) helper would probably be handy to make that test more readable (also using if()s rather than a switch()es when there's only a single case handled).

The test could also check that the port obtained in private browsing gets closed? Though I guess there's no way to do that short of trying to postMessage to it and checking for an exception?
Attachment #685036 - Flags: review?(gavin.sharp) → review+
If this can get verified on trunk, we'll take nomination for uplift up to and including mozilla-release/mozilla-esr17 for our upcoming 17.0.1 release going to build on Wednesday.

Comment 4

5 years ago
Created attachment 685884 [details] [diff] [review]
Updated tests based on feedback

Only tests are changed, so carrying r+ forward.  Landed as:

Attachment #685036 - Attachment is obsolete: true
Attachment #685884 - Flags: review+
Anthony, can we get some targeted PB/Social testing on this from trunk/inbound builds in preparation for approving for the re-spin tomorrow?
Keywords: qawanted


5 years ago
Duplicate of this bug: 809997
QA Contact: anthony.s.hughes

Comment 7

5 years ago
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
1. Clear cache
2. Start Private Browsing
3. Enable Social and log in to Facebook
4. Open some flyouts and some chats
5. Exit Private Browsing
6. Open about:cache
> 6KB entry in my cache: https://www.facebook.com/desktop/fbdesktop2/socialfox/fbworker.js.php
> Facebook Messenger is disabled
7. Turn on Facebook Messenger from toolbar
> Sidebar asks me to Log In
8. Click Log In
> Redirected to Facebook's login page
9. Log in to my facebook account
> Facebook sidebar loads activity feed and friends list, no open chat sessions are restored

Is this expected given the patch in this bug?

Also note that any Social session data (cache, chats, etc) are NOT exposed to the Private Browsing mode session if Social is active in Normal mode first.

FYI, this testing was conducted using the mozilla-inbound-debug build from today (2012-11-28).
Chat state not being maintained between the social sessions inside/outside of PB is expected behavior - we reload the provider entirely, and right now Facebook doesn't make any attempt to restore chats (that's somewhat orthogonal to this bug and is something they could do on their end).

The cache behavior is odd, but I don't think it's related to this bug. Can you file it as a new bug, Firefox::SocialAPI, CC ehsan/josh?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Can you file it as a new bug, Firefox::SocialAPI, CC ehsan/josh?

Thanks Gavin, will do. I think we can call this sufficient for 17.0.1 go-to-build. Flagging for verification in those candidate builds.
Keywords: qawanted → verifyme
For those interested, see bug 816202.
Comment on attachment 685884 [details] [diff] [review]
Updated tests based on feedback

[Approval Request Comment]
Bug caused by (feature/regressing bug #): present since social shipped in 17
User impact if declined: using social in normal mode after using it in PB mode will be broken
Testing completed (on m-c, etc.): has automated test, on mc, see comment 8
Risk to taking this patch (and alternatives if risky): low risk - affects social PB only, unlikely make things any worse 
String or UUID changes made by this patch: none
Attachment #685884 - Flags: approval-mozilla-release?
Attachment #685884 - Flags: approval-mozilla-beta?
Attachment #685884 - Flags: approval-mozilla-aurora?
Comment on attachment 685884 [details] [diff] [review]
Updated tests based on feedback

Taking this for the 17.0.1 respin.  Please land asap to ESR17 as well on both default and relbranch GECKO170_2012111914_RELBRANCH.
Attachment #685884 - Flags: approval-mozilla-release?
Attachment #685884 - Flags: approval-mozilla-release+
Attachment #685884 - Flags: approval-mozilla-esr17+
Attachment #685884 - Flags: approval-mozilla-beta?
Attachment #685884 - Flags: approval-mozilla-beta+
Attachment #685884 - Flags: approval-mozilla-aurora?
Attachment #685884 - Flags: approval-mozilla-aurora+

Comment 14

5 years ago
default branch: https://hg.mozilla.org/releases/mozilla-esr17/rev/72c7fb5c87ae
GECKO170_2012111914_RELBRANCH branch: https://hg.mozilla.org/releases/mozilla-esr17/rev/5a2f349fd763

default branch: https://hg.mozilla.org/releases/mozilla-release/rev/ef7dd6be6cdb
status-firefox17: affected → fixed
status-firefox-esr17: affected → fixed

Comment 15

5 years ago
beta: https://hg.mozilla.org/releases/mozilla-beta/rev/003e1843e1d8
aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/fedc6e02c271
status-firefox18: affected → fixed
status-firefox19: affected → fixed
In testing this against 17.0.1esr, Juan called out something which I'd like to get confirmation on.

1. Start with a new profile
2. Enter private browsing mode
3. Log in to Facebook
4. Activate Social through the activation page
5. Exit private browsing mode
6. View social prefs in about:config
> social.active = true
> Social elements are not active in the UI

Can someone please confirm if this is expected?
That's "expected" insofar as it wasn't intended to be affected by this patch. The fact that the "activation" of the feature that occurred in private browsing persists outside of private browsing is potentially problematic, but also doesn't apply to a per-window PB world, so I'm not sure it's worth worrying about now.
Okay, thank you Gavin.
Apart from what's already been called out in this bug, I confirm that this is behaving as expected in all branches.
status-firefox17: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
status-firefox20: fixed → verified
status-firefox-esr17: fixed → verified
Keywords: verifyme


5 years ago
tracking-firefox-esr17: ? → +
You need to log in before you can comment on or make changes to this bug.