1561015 - Pass through BrowsingContexts rather than mozIDOMWindowProxy in window provider/open window APIs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P2)

Description

[Kris Maglione [:kmag]]

Currently, window.open() and the related window provider and window watcher APIs pass around mozIDOMWindowProxy objects to use as the returned window value. This clearly won't work for remote windows, so these APIs need to deal in BrowsingContexts instead.

Ideally, some of them should return WindowProxyHolder objects, but as they're implemented in XPIDL, that currently isn't really an option. Landing bug 1463015 would make that much easier.

Once this is done, the APIs still won't support returning window proxies for remote windows, but it's still a necessary first step.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky,mossop

This is the first step in making it possible to return remote WindowProxy
objects from window.open() and related APIs.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 2 - Return BrowsingContext from openWindow2. r=bzbarsky

There are some unfortunate aspects of openWindow() and openWindow2() that make
this difficult. Namely, they sometimes return top-level chrome windows, and
sometimes a single content window from the top-level chrome window that they
open, depending on how they're called.

There really isn't any reason to return a BrowsingContext rather than a chrome
window in the former case, but there also really isn't a way to make the API
polymorphic in a way that would allow handling the two cases differently. So
at some point, the two cases should ideally be split into separate APIs rather
than separate special cases of a single API.

In the mean time, I've left openWindow() returning local mozIDOMWindowProxy
objects, since it isn't used by the DOM window.open() or openDialog() APIs,
and only updated openWindow2(). As a follow-up, we should remove both
openWindow() and openWindow2(), and replace them with openChromeWindow() and
openContentWindow() (or similar) methods which make it immediately clear what
type of window is being returned.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f7623a522b
Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky,mossop
https://hg.mozilla.org/integration/autoland/rev/156072a55a9a
Part 2 - Return BrowsingContext from openWindow2. r=bzbarsky

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1a - Consider requesting context in BrowsingContext lookup methods. r=nika

Access to a particular named browsing context depends on the caller who is
attempting the access. For a call to parent.open(..., name), for instance,
it's the privileges of the sub-frame making the open() call that matter, even
though the name resolution happens relative to the parent.

The current BrowsingContext FindWithName logic always considers only the
access of the BrowsingContext it's searching relative to, regardless of the
caller, while the corresponding DocShell logic correctly takes the caller into
account.

This patch updates the APIs to allow passing a specific accessing
BrowsingContext, and falls back to the target when one isn't passed (e.g., by
WebIDL callers, to which the new parameter is not exposed).

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1b - Fix FindWithSpecialName with inaccessible "_parent". r=nika

The special name "_parent" resolves to the parent window when one exists, or
the current target window if it does not. Either way, it returns null if the
target is inaccessible.

The current logic, however, treats inaccessible the same as nonexistent, and
returns the current window if it is has a parent from which it is sandboxed.
This differs from the corresponding DocShell logic, which returns null in that
case.

This patch aligns the BrowsingContext behavior with the DocShell behavior.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1c - Set BrowsingContext Closed flag at the same time as outer window's. r=nika

We rely on the Closed flag to avoid targeting named window open operations to
windows which have already closed. The DocShell's lookup logic checks the
Closed flag of the outer window, while BrowsingContext's checks the flag of
the context. The latter, however, is only set when the window's DocShell is
destroyed, which happens much later, and leaves closed windows returning true
from IsTargetable() for much longer than they should.

This patch immediately sets the BrowsingContext's closed flag at the same time
as we set the same flag on the outer window, and leaves the existing setters
in case of any corner cases.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1d - Move OnePermittedSandboxedNavigator to BrowsingContext. r=nika

We need to be able to check the one-permitted-sandboxed-navigator from
potentially-cross-process access checks in DocShell, which means it needs to
live on BrowsingContext rather than DocShell.

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1e - Use BrowsingContext as target in IsSandboxedFrom(). r=nika

This check always has access to an in-process DocShell which is attempting to
perform a load, but its target may often be cross-process, and have no
in-process DocShell. This patch changes the target checks to use a
BrowsingContext in a Fission-compatible way.

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1f - Implement BrowsingContext::LoadURI. r=nika

In order to do cross-process targeted window.open() and link click operations,
we need a way to load URIs in the current DocShell of a BrowsingContext,
whichever process it lives in.

This patch does this in the simplest possible way, bouncing the URL, along
with the target and accessor contexts, up to the parent and down to the
current owning child process. It does some basic sanity checks in the parent,
which should probably be expanded in the future, and should really ideally try
to initiate the load in the parent as soon as possible.

But for now, it does what we need.

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/36f7623a522b
https://hg.mozilla.org/mozilla-central/rev/156072a55a9a

Comment 11

[Jorg K (CEST = GMT+2)]

Looks like someone forgot "leave-open" on the bug. Will the various changes affect the windows interfaces we use in Thunderbird? In this context we note that nothing in bug 1463015 has landed and bug 1463016 is also still incomplete with Boris' NI open since January 2019 :-(

Kris, it would be really good if you could explain the broader picture here.

Comment 12

[Jorg K (CEST = GMT+2)]

"Will the various changes affect the windows interfaces we use in Thunderbird?" - Yes, it does, we're busted.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1561015: Part 1b.2 - Don't fall back to named lookup for inaccessible special name. r=nika

Currently, if a window with a special name is inaccessible to the caller, we
fall back to ordinary named lookup, which is not desirable.

This patch changes that behavior so that we never attempt fallback for special
names.