804653 - Make docshell trigger PB mode exit from Destroy, not destructor

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Josh Matthews [:jdm]]

This looks like it should eliminate all of the non-determinism involved in waiting for the "end of PB mode" notification, which is a Big Deal.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make destroying a private docshell update the global count of private docshells instead of waiting an indefinite amount of time for the destructor.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674295 [details] [diff] [review]
Make destroying a private docshell update the global count of private docshells instead of waiting an indefinite amount of time for the destructor.

Please set mInPrivateBrowsing first, before calling DecreasePrivateDocShellCount().  That would match the ordering in SetUsePrivateBrowsing.

r=me with that.

Comment 3

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #0)
> This looks like it should eliminate all of the non-determinism involved in
> waiting for the "end of PB mode" notification, which is a Big Deal.

What problems are caused by that?  Does this need branch landing?

Comment 4

[Josh Matthews [:jdm]]

The effects of this patch are that leaving PB mode doesn't have to wait on all private docshells being GCed, and can happen as soon as Destroy is called. I think a branch landing would be nice, since it might sidestep where we happen to be holding on to docshells and haven't noticed it while testing.

Comment 5

[(no longer active)]

(In reply to comment #4)
> The effects of this patch are that leaving PB mode doesn't have to wait on all
> private docshells being GCed, and can happen as soon as Destroy is called. I
> think a branch landing would be nice, since it might sidestep where we happen
> to be holding on to docshells and haven't noticed it while testing.

OK.  But what are the implications in practice?  Do you think that it can potentially cause data leakage?  (FWIW, I think that it can only caused some cleanup stuff to be called later than they probably should -- just want to make sure I'm not missing anything.)

Comment 6

[Josh Matthews [:jdm]]

Only if we've been negligent in the past would actual leakage occur. This shouldn't have any possible negative effects, though, and could have some positive ones. I'm leaning towards putting it on Aurora.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Only if we've been negligent in the past would actual leakage occur. This
> shouldn't have any possible negative effects, though, and could have some
> positive ones. I'm leaning towards putting it on Aurora.

Fair enough.  We should get it landed on central ASAP then.  Is the patch ready to go?

Comment 8

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5de2f1dd9d7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Sorry, but I had to back this out for leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c7a15c3b1c

https://tbpl.mozilla.org/php/getParsedLog.php?id=16585868&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 29656 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4 instances of BrowserNPObject with size 16 bytes each (64 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of Mutex with size 24 bytes each (48 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Preferences with size 64 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of PrincipalHolder with size 32 bytes

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Sorry, stupid mistake on my part. This patch had nothing to do with that. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/876a728f389a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/876a728f389a

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 674295 [details] [diff] [review]
Make destroying a private docshell update the global count of private docshells instead of waiting an indefinite amount of time for the destructor.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: Potential for privacy leaks in less well-tested parts of private browsing code due to private browsing cleanup not running for arbitrary periods of time.
Testing completed (on m-c, etc.): Landed on m-c, makes private browsing cleanup inordinately simpler to reason about in all tested cases so far. 
Risk to taking this patch (and alternatives if risky): Could break consumers who stop private browsing mode and then attempt to observe the last-pb-context-exited notification, but the likelihood of those existing is low (and they are incorrect, regardless).
String or UUID changes made by this patch: None.

Comment 13

[(no longer active)]

FWIW, I really recommend that we take this patch on Aurora, even though we're not yet aware of why we would need it (but it's best to not get surprised!)

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 674295 [details] [diff] [review]
Make destroying a private docshell update the global count of private docshells instead of waiting an indefinite amount of time for the destructor.

[Triage Comment]
Approving for Aurora 18 - we don't like surprises either. We'll get some exploratory PB testing around this bug just in case it does cause a regression (however unlikely).

Comment 15

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/573627e62736

Comment 16

[Manuela Muntean [Away]]

Josh, is there any visible effect of this bug fix ? What should QA be looking for when verifying this fix?

Comment 17

[Josh Matthews [:jdm]]

No, I am not aware of any situations in which this should cause visibly different changes.

Comment 18

[Manuela Muntean [Away]]

Ok, thank you Josh.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0

Verified on Linux, Mac and Windows for the request below. Verified basic private browsing operations. (cache, cookie, history, download clearence). Setting to verified.

(In reply to Alex Keybl [:akeybl] from comment #14)
> Comment on attachment 674295 [details] [diff] [review]
> Make destroying a private docshell update the global count of private
> docshells instead of waiting an indefinite amount of time for the destructor.
> 
> [Triage Comment]
> Approving for Aurora 18 - we don't like surprises either. We'll get some
> exploratory PB testing around this bug just in case it does cause a
> regression (however unlikely).