Privileged content process should be restricted to only loading pages that have URI_CAN_LOAD_IN_PRIVILEGED_CHILD

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
6 months ago
4 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

6 months ago
See summary. There might be some more comprehensive(?) things that we could do with the triggering principal here as well.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #0)
> See summary. There might be some more comprehensive(?) things that we could
> do with the triggering principal here as well.

Is this something we're going to work on soon, or should we pursue a workaround with a pref so that the about:blank pages are loaded in separate processes for the awsy(sb) tests?
Flags: needinfo?(mconley)
(Assignee)

Comment 2

5 months ago
(In reply to Eric Rahm [:erahm] from comment #1)
> Is this something we're going to work on soon, or should we pursue a
> workaround with a pref so that the about:blank pages are loaded in separate
> processes for the awsy(sb) tests?

I'm going to work on this next week, if that's alright - or is there a rush on this?
Flags: needinfo?(mconley) → needinfo?(erahm)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)
> (In reply to Eric Rahm [:erahm] from comment #1)
> > Is this something we're going to work on soon, or should we pursue a
> > workaround with a pref so that the about:blank pages are loaded in separate
> > processes for the awsy(sb) tests?
> 
> I'm going to work on this next week, if that's alright - or is there a rush
> on this?

It makes our AWSY results kind of unreliable... but next week is probably OK.
(In reply to Kris Maglione [:kmag] from comment #3)
> It makes our AWSY results kind of unreliable... but next week is probably OK.

Agreed, sooner is better than later. If it's going to slip further please let us know.
Flags: needinfo?(erahm)
(Assignee)

Comment 5

5 months ago
I'll try to take this today.
Assignee: nobody → mconley
(Assignee)

Comment 6

5 months ago
Before, we were allowing pages with nsIAboutModule.URI_CAN_LOAD_IN_CHILD, like
about:blank, load in the privileged content process.
(Assignee)

Comment 7

5 months ago
Ugh, a bunch of failing tests. :/ 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21931dfb7574b7269e4b10fdd02f09299361dd71

erahm, if you disable the preloaded about:newtab for your AWSY tests by setting browser.newtab.preload to false, does that cause the noise in AWSY to go away? That might be a faster path to success while I figure out what's going wrong here.
Flags: needinfo?(erahm)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #7)
> Ugh, a bunch of failing tests. :/ 
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=21931dfb7574b7269e4b10fdd02f09299361dd71
> 
> erahm, if you disable the preloaded about:newtab for your AWSY tests by
> setting browser.newtab.preload to false, does that cause the noise in AWSY
> to go away? That might be a faster path to success while I figure out what's
> going wrong here.

That seems to have made things slightly worse on windows [1], we we still end up with 3 about:blanks in the privileged process.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=52af79ef901dbf46ab5e7fcbac7f3c203d0055c8&framework=4&filter=win&showOnlyComparable=1&selectedTimeRange=172800
Flags: needinfo?(erahm)
(Assignee)

Comment 9

5 months ago
Okay, coming back to this then.
(Assignee)

Comment 10

5 months ago
So this original patch isn't going to work out because of how new tabs will often try to browse to about:blank early in their lifecycle before loading the actual requested content. (Gijs is well aware of this class of problem, I believe, due to his work in bug 1493606).

So with this patch, we can get into a case where a new about:newtab page first loads about:blank, so it process flips to the unprivileged process type, and then it tries to load about:newtab, so it process flips again to the privileged process type, and then it tries to go to about:blank again, ad infinitum. :/

I'll look at alternative solutions.
(Assignee)

Updated

4 months ago
Blocks: 1508577
No longer blocks: 1472212
(Assignee)

Updated

4 months ago
Blocks: 1513045
No longer blocks: 1508577
(Assignee)

Comment 11

4 months ago
So the solution I'm looking at right now is to special-case about:blank from within the privileged content process. I'm hoping I can convince it to load in the default web process if about:blank is navigated to via the parent process. I'm hoping about:blank can be loaded in the privileged content process only when the navigation starts from within the content process.

(We need the privileged content process to be able to load about:blank's, since the content process will often create an about:blank content viewer early in its lifetime).
(Assignee)

Comment 12

4 months ago
I have this mostly working, but I think I broke the file URI process affinity stuff (the code that keeps the first domain browsed to from a local file within the local file process, but any other domains cause a process flip). So digging into that now.
(Assignee)

Comment 13

4 months ago
I'm bailing out of this in favour of bug 1515201 instead.

I think it's still true that we need to ensure that only the right pages are loaded in the privileged content process, but I don't think we can practically remove about:blank from that list. Not yet, anyhow.

I'm going to WONTFIX this for now, I guess.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → WONTFIX
Attachment #9025494 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.