829383 - gNumberOfAlwaysOpenPrivateDocShells will be incorrect when hidden private window contains child docshells

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Josh Matthews [:jdm]]

Won't the number of always open private docshells be equal to the 0 or 1 + the number of child docshells of the hidden private window? This looks like it will break once Jetpack starts adding panels to it.

Comment 1

[(no longer active)]

As discussed on IRC, let's store a flag on the docshell, something like mDontGiveACrap (with a better name), set it to true on the hidden window docshell, propagate it to children, and bypass this whole business of increasing and decreasing the private docshell count when that flag is set...

We should also get a test for this.  You can stick an iframe or something under the private hidden window...

Comment 2

[Josh Matthews [:jdm]]

Attachment: Ensure hidden private window docshells aren't counted towards private session lifetime.

Every time I change this code I find cause to feel slightly dirtier.

Comment 3

[(no longer active)]

Comment on attachment 702337 [details] [diff] [review]
Ensure hidden private window docshells aren't counted towards private session lifetime.

Let's assume that Josh has rev'ed nsIDocShell's uuid.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 702337 [details] [diff] [review]
Ensure hidden private window docshells aren't counted towards private session lifetime.

Don't you need to IncreasePrivatDocShellCount() if aAffectLifetime && !mAffectPrivateSessionLifetime && mInPrivateBrowsing?

r=me with that fixed.

Comment 5

[Josh Matthews [:jdm]]

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

Comment 6

[Josh Matthews [:jdm]]

This is going to affect 20, so it will need an uplift.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 702337 [details] [diff] [review]
Ensure hidden private window docshells aren't counted towards private session lifetime.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 815847
User impact if declined: Private browsing data that doesn't get cleared when using certain jetpack addons.
Testing completed (on m-c, etc.): m-c, automated test
Risk to taking this patch (and alternatives if risky): Not risky. Well-scoped functionality change to one component.
String or UUID changes made by this patch: nsIDocShell UUID

Comment 8

[Josh Matthews [:jdm]]

Backed out for failing OS X test: https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa6bf0a46a2

Comment 9

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=53506e00cbf4

Comment 10

[Josh Matthews [:jdm]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702337 [details] [diff] [review]
Ensure hidden private window docshells aren't counted towards private session lifetime.

Approving for aurora since we need/want this for the pb feature in 20.

Comment 14

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/326cf46aa524

Comment 15

[(no longer active)]

The test landed here only makes sense in per-window PB builds.  This patch marks it as such:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e775b0323811
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1c12aa591e0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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