795556 - Propagate parent window privacy status to newly opened windows

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Josh Matthews [:jdm]]

Bug 795015 is currently doing this in the frontend, and that is not great. It looks like we should be doing this properly in nsWindowWatcher::OpenWindowInternal, where we have parentTreeOwner and newDocShellItem that we can manipulate.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Move privacy status propagation for new windows into the platform.

This passes the relevant tests and fixes the problem described in bug 795015.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666185 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.

r=me, though I wonder whether we should propagate even if !windowIsNew...

Comment 3

[(no longer active)]

I think this can land now.

Comment 4

[Josh Matthews [:jdm]]

I need to look into the !windowIsNew case first.

Comment 5

[Josh Matthews [:jdm]]

I've read through nsContentTreeOwner::ProvideWindow and the rest of nsWindowWatcher::OpenWindowInternal, and it looks to me like in all cases where !windowIsNew, we're doing the open in such a way that there is an established docshell hierarchy that will cause the privacy status to be propagated.

Comment 6

[Josh Matthews [:jdm]]

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

I realized I should really get rid of the nsDocShell code that replicates the global PB state, and I threw an assertion in to catch any places where we don't match up with the global state.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Move privacy status propagation for new windows into the platform. * *

try: -b d -p all -u mochitests -t none

Comment 8

[Josh Matthews [:jdm]]

Attachment: Move privacy status propagation for new windows into the platform. * *

Fun times. We need the flag propagation in both the app shell service and the window watcher because in cases like OOP tabs an in-process mozbrowser windows we don't flow through the app shell.

Comment 9

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=155d4a0fe75a

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 668563 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.  * *

Test failures on try.

Comment 11

[Josh Matthews [:jdm]]

Ehsan, I have discovered that I receive assertion failures in the attached patch unless I change the loop in nsPrivateBrowsingService to set the privacy flag on all windows, not just navigator:browser. This makes sense, since the docshell propagation code doesn't distinguish between this case. Your thoughts?

Comment 12

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #11)
> Ehsan, I have discovered that I receive assertion failures in the attached
> patch unless I change the loop in nsPrivateBrowsingService to set the
> privacy flag on all windows, not just navigator:browser. This makes sense,
> since the docshell propagation code doesn't distinguish between this case.
> Your thoughts?

That's fine by me.

Comment 13

[(no longer active)]

Note to myself: there seems to be an assertion triggered in browser_styleeditor_private.js.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Move privacy status propagation for new windows into the platform. * *

try: -b d -p all -u mochitests -t none

Comment 15

[Josh Matthews [:jdm]]

Attachment: Part 1: Make Style Editor open with sensible parent window.

Comment 16

[Josh Matthews [:jdm]]

Attachment: Part 2: Move privacy status propagation for new windows into the platform.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 669661 [details] [diff] [review]
Part 2: Move privacy status propagation for new windows into the platform.

r=me

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 669658 [details] [diff] [review]
Part 1: Make Style Editor open with sensible parent window.

A content window as the parent of a chrome window doesn't seem very sensible to me...

Comment 19

[Josh Matthews [:jdm]]

Would the window that is the root item of the docshell tree be a better choice?

Comment 20

[Dão Gottwald [:dao]]

newEditor and toggleEditor are called from browser.js -- we could just pass the chrome window from there in addition to the content window.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Part 1: Make Style Editor open with sensible parent window.

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 669738 [details] [diff] [review]
Part 1: Make Style Editor open with sensible parent window.

Review of attachment 669738 [details] [diff] [review]:
-----------------------------------------------------------------

I can't say I'm a fan of "|=", but it's not a violent loathing, so I'm not going to fight it.

Comment 23

[(no longer active)]

Josh, is this good to go?  I didn't push it since I got the impression that you're working to make the patches better...

Comment 24

[Josh Matthews [:jdm]]

This is ready to go.

Comment 25

[Josh Matthews [:jdm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ef4a2d8caa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27e8924be2c1

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a0ef4a2d8caa
https://hg.mozilla.org/mozilla-central/rev/27e8924be2c1