Closed Bug 1561015 Opened 1 year ago Closed 1 year ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

Fission Milestone: --- → ?

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

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.

Status: NEW → ASSIGNED
Fission Milestone: ? → M4
Priority: -- → P2
Blocks: fission-mochitests
No longer blocks: fission
Blocks: 1562292
Attachment #9073612 - Attachment description: Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=nika → Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky
Attachment #9073612 - Attachment description: Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky → Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky,mconley
Attachment #9073613 - Attachment description: Bug 1561015: Part 2 - Return BrowsingContext from openWindow2. r=nika → Bug 1561015: Part 2 - Return BrowsingContext from openWindow2. r=bzbarsky
Attachment #9073612 - Attachment description: Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky,mconley → Bug 1561015: Part 1 - Use BrowsingContext in window provider APIs. r=bzbarsky,mossop
Pushed by maglione.k@gmail.com:
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

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).

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.

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.

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.

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.

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.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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.

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

Regressions: 1571272
Regressions: 1571203

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.

You need to log in before you can comment on or make changes to this bug.