1522546 - Not much preventing confusion between ContentProcess mRemoteType and the remoteType set on associated <browser>'s

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Filing this as a sec bug just out of an abundance of caution, but this is mostly theoretical at this point.

In bug 1509250, I've been fiddling with a patch that attempts to make the initial browser in a new window use the privileged content process in the event that it hasn't been told yet to go anywhere else. This is to avoid a potential process flip if we're still early in start-up, and we haven't yet determine that we should send the initial tab to about:home or not (where about:home is the common case).

I pushed the patch to try and got a bunch of test failures, and those failures tend to come from tests that open new windows. One in particular, browser_new_content_window_from_chrome_principal.js, uses a ContentTask to call content.open() in privileged scope on a normal content process.

Here's what appears to happen:

1. The TabChild is created first on the content process's side, and then sent up to the parent process to request that it be hosted in a new window
2. The parent receives the message, does the needful to construct a new window.
3. The new window is constructed, and (with the patch I linked to), the initial browser is set up to have remoteType set to "privileged".
4. The nsFrameLoader code kicks in as soon as the browser is put into the DOM, and it connects the TabParent paired with the content process's TabChild to the <browser> element. No new process is created - the TabChild is running in the same content process that originally requested the new window.

The end result is that we have a content process that has two associated <browser> elements, but one of which has remoteType="privileged" on it. Thankfully, content doesn't think it's privileged, and the ContentParent doesn't think it's privileged, but any code that might check the remoteType of the <browser> might get confused about what rights that browser has.

I think this kinda highlights the fragility with how we set browser "remote types" here. Right now, when the front-end wants to say what type of process a <browser> should be associated with, it sets that remoteType attribute on the <browser> itself, and we just kinda hope that it reflects reality.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Cc'ing bobowen, because I know he's been involved with the different remoteType's for browsers, like file://.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Nika, did you have any thoughts on this? With the process-flipping stuff that was coming down the pike for Fission, were there thoughts on how process type transitions were going to work?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

My plan with fission process changes is to eventually change remoteType to not be controlled by frontend code at all, instead being a property of the internal FrameLoader which can change over time. The remote and remoteType attributes on the element will probably disappear entirely soon™.

At one point in time I had added some special logic to FrameLoader to mutate the remoteType attribute after connecting an existing TabParent, but it seems like I never landed it.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Nika Layzell from comment #3)

At one point in time I had added some special logic to FrameLoader to mutate the remoteType attribute after connecting an existing TabParent, but it seems like I never landed it.

I can probably hack a similar patch together if we think that'll be sufficient in the short-term to avoid confusion. I do look forward to the day where the front-end doesn't need to declare process type.

Comment 5

[Kris Maglione [:kmag]]

Why exactly are the tests failing?

I agree that the remoteType attribute on the <browser> can be confusing if it doesn't match the remote type of the child, but nothing should really be relying on that. Anyone who wants to know the remoteType of a browser should be checking the remoteType property of its message manager.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kris Maglione [:kmag] from comment #5)

Why exactly are the tests failing?

The test is failing because the BrowserTestUtils code that waits for the new window to open here is expecting that, since the initial browser has remoteType set to "privileged", there's going to be a remoteness flip to load the requested URL, so it sets up XULFrameLoaderCreated handler and waits for it.

But the handler is never fired, since there is never a remoteness flip - the first <browser> inserted is properly pointed at the right content process.

So here's an example of some code that's confused by browser.remoteType. It's in test code, but there are other uses of browser.remoteType around and that makes me a little antsy.

Perhaps we should have browser.remoteType alias to browser.messageManager.remoteType for convenience, and always remove the remoteType attribute from the <browser> after it has been inserted into the DOM to avoid confusion.

Would that be better?

Comment 7

[Kris Maglione [:kmag]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

So here's an example of some code that's confused by browser.remoteType. It's in test code, but there are other uses of browser.remoteType around and that makes me a little antsy.

Let's get rid of those uses, then.

Perhaps we should have browser.remoteType alias to browser.messageManager.remoteType for convenience, and always remove the remoteType attribute from the <browser> after it has been inserted into the DOM to avoid confusion.

Would that be better?

Probably.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kris Maglione [:kmag] from comment #7)

Would that be better?

Probably.

Alright cool, then that's what I'm going to do. Thanks. :)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Just an update - I have some patches to fix this, but I'm just dealing with one piece of testing fallout that I can't reproduce locally. Classic. :/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Make tabbrowser access browser remoteType property rather than the attribute. r?Gijs

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Make remoteType property on browsers forward to the message manager if applicable. r?bgrins

Depends on D18223

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Make GeckoView use browser.remoteType instead of getting at the attribute directly. r?snorp

Depends on D18224

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Properly handle preferred remote types in BrowserTestUtils.waitForNewWindow. r?bobowen

This is to fix some of our tests that use BrowserTestUtils.waitForNewWindow, where
the browser that ends up being passed to it doesn't actually need to flip
remoteness.

For example, in the file:// URI case, we allow the first browse to an HTTP
URI to run within the same process. This means that the preferred remote
type is "file", despite the URI normally mapping to the "web" type of
content process.

Depends on D18225

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Make RDM swap code follow the browser binding on how remoteType shortcuts to the message manager. r?ochameau

Depends on D18226

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1522546 - Drop remoteType attribute on frame elements after bound to a ContentParent. r?nika

Depends on D18227

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad1efb54b6a9f43b1621af90c1caa10817d727d

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6dd3f2bb4145
https://hg.mozilla.org/mozilla-central/rev/87036e8a2433
https://hg.mozilla.org/mozilla-central/rev/dbe46aad4cbf
https://hg.mozilla.org/mozilla-central/rev/3de692f73e2f
https://hg.mozilla.org/mozilla-central/rev/e745963cb907
https://hg.mozilla.org/mozilla-central/rev/e39bd319fe3e

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/6dd3f2bb4145b28f719f948e544fc1acd0a56b9a
https://hg.mozilla.org/integration/autoland/rev/87036e8a2433e563dba18159f99bf791ffdc7952
https://hg.mozilla.org/integration/autoland/rev/dbe46aad4cbfab52e7e187ecec87b526fc4c392f
https://hg.mozilla.org/integration/autoland/rev/3de692f73e2f5a6eb0c58eae848b2b80b5d28214
https://hg.mozilla.org/integration/autoland/rev/e745963cb9074c64c381cd58f9dff8a39d9d9cd2
https://hg.mozilla.org/integration/autoland/rev/e39bd319fe3e2d8e310467fa65bc2a8eab398992