Closed Bug 795556 Opened 7 years ago Closed 7 years ago

Propagate parent window privacy status to newly opened windows

Categories

(Core :: DOM: Navigation, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- disabled

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Assignee: nobody → josh
This passes the relevant tests and fixes the problem described in bug 795015.
Attachment #666185 - Flags: review?(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...
Attachment #666185 - Flags: review?(bzbarsky) → review+
I think this can land now.
I need to look into the !windowIsNew case first.
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.
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.
try: -b d -p all -u mochitests -t none
Attachment #666185 - Attachment is obsolete: true
Blocks: 798508
No longer blocks: 798508
Blocks: 798508
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.
Attachment #668563 - Flags: review?(bzbarsky)
Attachment #668548 - Attachment is obsolete: true
Comment on attachment 668563 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.  * *

Test failures on try.
Attachment #668563 - Flags: review?(bzbarsky)
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?
(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.
Note to myself: there seems to be an assertion triggered in browser_styleeditor_private.js.
try: -b d -p all -u mochitests -t none
Attachment #668563 - Attachment is obsolete: true
Attachment #669658 - Flags: review?(jwalker)
Attachment #669543 - Attachment is obsolete: true
Comment on attachment 669661 [details] [diff] [review]
Part 2: Move privacy status propagation for new windows into the platform.

r=me
Attachment #669661 - Flags: review?(bzbarsky) → review+
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...
Would the window that is the root item of the docshell tree be a better choice?
newEditor and toggleEditor are called from browser.js -- we could just pass the chrome window from there in addition to the content window.
Attachment #669658 - Attachment is obsolete: true
Attachment #669658 - Flags: review?(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.
Attachment #669738 - Flags: review?(jwalker) → review+
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...
This is ready to go.
https://hg.mozilla.org/mozilla-central/rev/a0ef4a2d8caa
https://hg.mozilla.org/mozilla-central/rev/27e8924be2c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 801151
Depends on: 813533
You need to log in before you can comment on or make changes to this bug.