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 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.

(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?

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.

(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.)

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.

(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?

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

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 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.

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 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).

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

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

Ok, thank you Josh.

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).