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.
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.
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?
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.
Created attachment 685884 [details] [diff] [review] Updated tests based on feedback Only tests are changed, so carrying r+ forward. Landed as: https://hg.mozilla.org/integration/mozilla-inbound/rev/db5b030c100a
Anthony, can we get some targeted PB/Social testing on this from trunk/inbound builds in preparation for approving for the re-spin tomorrow?
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 email@example.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.
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
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.
esr17: default branch: https://hg.mozilla.org/releases/mozilla-esr17/rev/72c7fb5c87ae GECKO170_2012111914_RELBRANCH branch: https://hg.mozilla.org/releases/mozilla-esr17/rev/5a2f349fd763 release: default branch: https://hg.mozilla.org/releases/mozilla-release/rev/ef7dd6be6cdb
beta: https://hg.mozilla.org/releases/mozilla-beta/rev/003e1843e1d8 aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/fedc6e02c271
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.