Make docshell trigger PB mode exit from Destroy, not destructor

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 verified)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

Assignee

Description

7 years ago
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.
Assignee

Updated

7 years ago
Blocks: 722859
Assignee

Updated

7 years ago
Assignee: nobody → josh
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.
Attachment #674295 - Flags: review?(bzbarsky) → review+

Comment 3

7 years ago
(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?
Assignee

Comment 4

7 years ago
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

7 years ago
(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.)
Assignee

Comment 6

7 years ago
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

7 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/876a728f389a
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 12

7 years ago
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.
Attachment #674295 - Flags: approval-mozilla-aurora?

Comment 13

7 years ago
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).
Attachment #674295 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: qawanted, verifyme
Josh, is there any visible effect of this bug fix ? What should QA be looking for when verifying this fix?
Assignee

Comment 17

7 years ago
No, I am not aware of any situations in which this should cause visibly different changes.
Whiteboard: [qa-]
Ok, thank you Josh.
Keywords: qawanted, verifyme
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).
QA Contact: virgil.dicu
You need to log in before you can comment on or make changes to this bug.