Closed Bug 1885598 Opened 3 months ago Closed 3 months ago

Improve and Clean up some privileged-mozilla-content-process code

Categories

(Core :: DOM: Navigation, defect)

defect

Tracking

()

RESOLVED FIXED
126 Branch
Tracking Status
firefox126 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(3 files, 2 obsolete files)

While working to get a resource:// URI loading in the privileged mozilla content process, I stumbled around and made the following observations I made patches for.

1: I think, but am not 100% certain, that the parameter name given in PageDataService is incorrect and being ignored. I haven't really tested the code, I just noticed this when my code, that I copied from there, was wrong. It's saying remoteType when it should be remoteTypeOverride. That said, it probably shouldn't be there at all (see (3 below).

2: Passing remoteTypeOverride for anything other than an about: page is dislloawed by spec and enforced.

3: This surprised me, because I was fiddling with the E10sUtils code for privilegedmozilla, making it support resource:// urls in addition to https. My code was passing through this function (lazy.E10SUtils.getRemoteTypeForURIObject to browser.loadURI) and was failing in the aforementioned remoteType restriction.

It seems like getRemoteTypeForURI[Object] is most commonly used for addTab, createBrowser although PageDataService is a notable exception. There is also HeadlessShell.sys.mjs which does the lazy.E10SUtils.getRemoteTypeForURIObject to browser.loadURI pattern and might be wrong.

I did see 30 hits for the return PRIVILEGEDMOZILLA_REMOTE_TYPE; line in E10SUtils which made me suspicious, but I believe that is browser_e10s_mozillaweb_process.js and browser_privilegedmozilla_process_pref.js which notably don't navigate the browser to those domains, they just check the expected value from parsing.

lazy.E10SUtils.getRemoteTypeForURIObject feels like a footgun when it looks like it can be used with browser.loadURI. Or possibly I just did a triple-gainer-to-slug-in-foot and it's not that common of a concern. But I wonder if there's an assertion we can put somewhere

I don't know if there is a path from E10SUtils.getRemoteTypeForURIObject into loading a URI that doesn't path through browser.loadURI - but at best it seems like this is a footgun. I'm going to experiment with putting an assertion at that remoteType check in DocumentLoadListener.

4: After figuring out that the code I put into E10SUtils wasn't doing what I wanted (well, it was returning the correct remoteType, but I couldn't feed it in where I expected), I found the code duplicated in ProcessIsolation.cpp and updated it there also.

5: Completely unrelated to the above, my first attempt was to implement process restrictions using remoteTypes: ["webIsolated=resource://path/file.html"] when registering the actor. This didn't work, and it has the limitation that it will only work for Desktop when Fission is enabled, but I kind of think it should work? Or maybe I'm just writing a footgun for someone else on Android.

Firstly, this parameter is actually named remoteTypeOverride and
secondly, it is only valid for about: urls when passed to loadURI

Admittedly, this won't work on Android or non-Fission users
but perhaps it has a place for certain Desktop actors.

It will still disallow http, but this will permit it to load resource://
URIs in particular. The URI must still be present in the appropriate
pref.

Severity: -- → S3
Attachment #9391500 - Attachment is obsolete: true
See Also: → 1885991
Attachment #9391501 - Attachment is obsolete: true
Attachment #9391502 - Attachment description: WIP: Bug 1885598: Assert on a passed remoteTypeOverride to avoid footguns → Bug 1885598: Assert on a passed remoteTypeOverride to avoid footguns r?nika
Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa60e8a2dd07
Do not pass remoteType r=mossop
https://hg.mozilla.org/integration/autoland/rev/0669743f23df
Add a couple of pointer comments to duplicated code r=nika
https://hg.mozilla.org/integration/autoland/rev/993eba2cdb4f
Assert on a passed remoteTypeOverride to avoid footguns r=nika
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 126 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: