Last Comment Bug 815042 - leaving private browsing with social enabled doesn't reset all social components
: leaving private browsing with social enabled doesn't reset all social components
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 20
Assigned To: Mark Hammond [:markh]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
: 809997 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-25 21:51 PST by Mark Hammond [:markh]
Modified: 2012-11-29 16:16 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
+
verified
+
verified


Attachments
Reset all social components as PB exits. (4.39 KB, patch)
2012-11-25 21:51 PST, Mark Hammond [:markh]
gavin.sharp: review+
Details | Diff | Review
Updated tests based on feedback (4.89 KB, patch)
2012-11-27 16:31 PST, Mark Hammond [:markh]
markh: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑release+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Review

Description Mark Hammond [:markh] 2012-11-25 21:51:37 PST
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.
Comment 1 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-26 09:57:47 PST
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 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-27 12:02:08 PST
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?
Comment 3 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-27 13:46:31 PST
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 Mark Hammond [:markh] 2012-11-27 16:31:03 PST
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
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-27 16:53:03 PST
Anthony, can we get some targeted PB/Social testing on this from trunk/inbound builds in preparation for approving for the re-spin tomorrow?
Comment 6 Mark Hammond [:markh] 2012-11-27 20:08:41 PST
*** Bug 809997 has been marked as a duplicate of this bug. ***
Comment 7 Ed Morley [:emorley] 2012-11-28 10:33:29 PST
https://hg.mozilla.org/mozilla-central/rev/db5b030c100a
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-28 10:49:32 PST
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).
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-28 11:46:54 PST
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?
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-28 11:49:43 PST
(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.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-28 11:56:33 PST
For those interested, see bug 816202.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-28 15:13:48 PST
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 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-28 15:27:41 PST
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.
Comment 14 Mark Hammond [:markh] 2012-11-28 16:09:52 PST
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
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-29 14:10:33 PST
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?
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-29 14:16:00 PST
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.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-29 14:19:10 PST
Okay, thank you Gavin.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-29 15:06:44 PST
Apart from what's already been called out in this bug, I confirm that this is behaving as expected in all branches.

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