Propagate parent window privacy status to newly opened windows

RESOLVED FIXED in mozilla19

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 disabled)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Assignee: nobody → josh
(Assignee)

Comment 1

5 years ago
Created attachment 666185 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.

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+

Comment 3

5 years ago
I think this can land now.
(Assignee)

Comment 4

5 years ago
I need to look into the !windowIsNew case first.
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

5 years ago
Created attachment 668548 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.  * *

try: -b d -p all -u mochitests -t none
(Assignee)

Updated

5 years ago
Attachment #666185 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 798508
No longer blocks: 798508
Blocks: 798508
(Assignee)

Comment 8

5 years ago
Created attachment 668563 [details] [diff] [review]
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.
Attachment #668563 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #668548 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=155d4a0fe75a
(Assignee)

Comment 10

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

Comment 11

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

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

5 years ago
Note to myself: there seems to be an assertion triggered in browser_styleeditor_private.js.
(Assignee)

Comment 14

5 years ago
Created attachment 669543 [details] [diff] [review]
Move privacy status propagation for new windows into the platform.  * *

try: -b d -p all -u mochitests -t none
(Assignee)

Updated

5 years ago
Attachment #668563 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 669658 [details] [diff] [review]
Part 1: Make Style Editor open with sensible parent window.
Attachment #669658 - Flags: review?(jwalker)
(Assignee)

Comment 16

5 years ago
Created attachment 669661 [details] [diff] [review]
Part 2: Move privacy status propagation for new windows into the platform.
Attachment #669661 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 19

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

Comment 21

5 years ago
Created attachment 669738 [details] [diff] [review]
Part 1: Make Style Editor open with sensible parent window.
Attachment #669738 - Flags: review?(jwalker)
(Assignee)

Updated

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

Comment 23

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

Comment 24

5 years ago
This is ready to go.
(Assignee)

Comment 25

5 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ef4a2d8caa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27e8924be2c1
https://hg.mozilla.org/mozilla-central/rev/a0ef4a2d8caa
https://hg.mozilla.org/mozilla-central/rev/27e8924be2c1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Depends on: 801151

Updated

5 years ago
Depends on: 813533

Updated

5 years ago
status-firefox19: --- → disabled
Depends on: 814275
You need to log in before you can comment on or make changes to this bug.