Not much preventing confusion between ContentProcess mRemoteType and the remoteType set on associated <browser>'s
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
Details
(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main67-])
Attachments
(6 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1522546 - Properly handle preferred remote types in BrowserTestUtils.waitForNewWindow. r?bobowen
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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:
- 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
- The parent receives the message, does the needful to construct a new window.
- The new window is constructed, and (with the patch I linked to), the initial browser is set up to have remoteType set to "privileged".
- 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.
Assignee | ||
Comment 1•6 years ago
|
||
Cc'ing bobowen, because I know he's been involved with the different remoteType's for browsers, like file://.
Assignee | ||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
(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 existingTabParent
, 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•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
(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•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
(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. :)
Assignee | ||
Comment 9•6 years ago
|
||
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. :/
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
Depends on D18223
Assignee | ||
Comment 12•6 years ago
|
||
Depends on D18224
Assignee | ||
Comment 13•6 years ago
|
||
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
Assignee | ||
Comment 14•6 years ago
|
||
Depends on D18226
Assignee | ||
Comment 15•6 years ago
|
||
Depends on D18227
Assignee | ||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
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•6 years ago
|
||
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
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•4 years ago
|
Description
•