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

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

Attachment: 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]

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: [e-mail]]]

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]

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] [:mhammond]]

Attachment: 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: [e-mail]]]

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 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db5b030c100a

Comment 8

[u279076]

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: [e-mail]]]

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

[u279076]

(In reply to :Gavin Sharp (use [e-mail] 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

[u279076]

For those interested, see bug 816202.

Comment 12

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

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]

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] [:mhammond]]

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 15

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

beta: https://hg.mozilla.org/releases/mozilla-beta/rev/003e1843e1d8
aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/fedc6e02c271

Comment 16

[u279076]

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: [e-mail]]]

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

[u279076]

Okay, thank you Gavin.

Comment 19

[u279076]

Apart from what's already been called out in this bug, I confirm that this is behaving as expected in all branches.