1690100 - [META] Set BrowsingContext flags from the parent process

Status: NEW

(DevTools :: General, task, P3)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1662840 +++

The functions which set these flags currently try to propagate them to descendants using nsDocumentViewer::PropagateToPresContextsHelper, which only handle in-process children. We want to handle out-of-process children too, which means we need to use store them on WindowContext or BrowsingContext instead, and propagate them using something like BrowsingContext::PreOrderWalk.

We should then remove PropagateToPresContextsHelper.

Note that this involves adding DidSet* hooks to BrowsingContext or WindowContext for each of these flags to update the layout state of the child process they live in when the flags change.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Is this different to bug 1591120? I was planning to look at that soonish, and this was basically the approach I was going to take.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Is this different to bug 1591120? I was planning to look at that soonish, and this was basically the approach I was going to take.

No, it looks like the same bug, aside from not including EmulateMedium(). I'd add that the flags should probably only be settable in the parent process, though.

Comment 3

[Julian Descottes [:jdescottes]]

Kris, Emilio: it looks like EmulateMedium was also handled in bug 1591120 ?
I guess what remains is that we should only set them in the parent process now?

Do you confirm, should I rename & repurpose this?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, sounds good.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Kris, Emilio: it looks like EmulateMedium was also handled in bug 1591120 ?
I guess what remains is that we should only set them in the parent process now?

Do you confirm, should I rename & repurpose this?

Yeah. Same for the other BrowsingContext properties set by RDM in the content process.

Comment 6

[Neha Kochar [:neha]]

Honza, can you please get this prioritized, as this blocks another Fission M7 bug? This is currently marked for dt-future and that won't be done in time for Fission M7 (Fx88).

Comment 7

[Julian Descottes [:jdescottes]]

On DevTools side, since Bug 1690698 landed the configuration first goes to the parent process before being forwarded to the content process.
So for all the flags which are currently set in browsing-context.js updateConfiguration (searchfox), the DevTools part should be relatively easy to implement (eg for color scheme & print media simulation).

Same for the other BrowsingContext properties set by RDM in the content process.

For RDM properties, sadly we don't use a configuration actor. So all the properties go directly to the content process at the moment. We'll either need to send them to the target configuration actor or come up with a new actor if we feel like this should be separated from the target config.

Let's start by reviewing the list of all flags/methods we use on BrowsingContext to know what needs to be migrated:

• overrideDPPX (RDM)
• touchEventsOverride (RDM)
• watchedByDevTools (BC target actor)
• serviceWorkersTestingEnabled (BC target actor)
• mediumOverride (BC target actor)
• prefersColorSchemeOverride (BC target actor)
• setRDMPaneMaxTouchPoints() (RDM)
• setRDMPaneOrientation() (RDM)
• allowJavascript (BC target actor, set on docShell)
• defaultLoadFlags (BC target actor, set on docShell)
• metaViewportOverride (RDM, set on docShell)
• customUserAgent (this one is sometimes set on DocShell (RDM) sometimes on browsingContext (webextensions))

Comment 8

[Julian Descottes [:jdescottes]]

For reference here's a POC moving 3 BC target actor flags to the parent process https://hg.mozilla.org/try/rev/e074989b0e5a9d9dc70aa9274cd9cca770876bec

It's not clear where the parent process code to apply the flags should live, but something tied to DevtoolsFrameParent feels like a decent choice because we would need those settings to be applied to subsequent targets as well.

Updating the flags seems to work fine, but the cleanup code is not working at all with this patch. And it doesn't seem trivial to fix it.

Today we are not destroying DevToolsFrameParent correctly when the toolbox closes. This is already mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1691661#c0. It would be slightly improved by fixing holes in our logic to unregister jswindowactors (eg https://phabricator.services.mozilla.com/D105545), but still won't cover everything.

And for targets created from the client (ie all our top level targets at the moment), it is even worst. The JsWindowActor pair is only created as a side effect when using addWatcherDataEntry. Which means that the child actor is not even listening to devtools-server-connection changes, and cannot react to the toolbox closing.

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #7)

Let's start by reviewing the list of all flags/methods we use on BrowsingContext to know what needs to be migrated:

• overrideDPPX (RDM)
  ...
• customUserAgent (this one is sometimes set on DocShell (RDM) sometimes on browsingContext (webextensions))

Emilio, Kris, can someone help us identify which flags/methods are mandatory to move to the parent process here? The bug was just opened for EmulateMediumInternal/EmulatePrefersColorSchemeInternal but then Comment 5 also mentions flags set from RDM.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Everything that might have some security impact probably should. In particular, serviceWorkerTestingEnabled should probably only be allowed to be set on the parent because it bypasses security checks. For the rest I have less strong opinions I think but not sure what Kris's thoughts are.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Alex, can you please perform some analysis for this bug? What would be the scope of work here?
We already have a list of M7 blockers and I am not sure whether we have a free slot there.
If not, it should definitely block M8.

Thank you!
Honza

Comment 12

[Kris Maglione [:kmag]]

At the minimum, any property that currently has a LegacyCheckOnlyOwningProcessCanSet check in BrowsingContext.cpp needs to be moved either to the parent process or to WindowContext, since those checks are racy, and the content process has no reason to need to set any of those properties. But all of the BrowsingContext properties set by RDM really need to move to the parent process. The ones that don't have the LegacyCheckOnlyOwningProcessCanSet check right now are only missing it because because the raciness makes it dangerous.

As Emilio says, any property that could have an impact on security is especially important, but the security impacts are not always obvious. I worry about overrideDPPX potentially being used for clickjacking in cross-origin frames, for instance, and there may be potentially interesting ways for customUserAgent to be abused.

Comment 13

[Neha Kochar [:neha]]

Honza, we need this fixed in Devtools latest by M8.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Neha Kochar [:neha] from comment #13)

Honza, we need this fixed in Devtools latest by M8.

Sounds fair. We have a few architectural changes which should hopefully make this possible without gross workarounds.
Bug 1644397 which should land soon, but probably not for M7, is a foundation stone.
Bug 1686748 will be the one true enabler.

I'll try to lay down the road for having this fixed for M8.

To be more concrete, I would suggest:

• making TargetConfiguration become a command (nice to have, not a blocker)
• have all frame targets be created by the Watcher and using JSWindowActor API (instead of message manager) [=bug 1686748]. Clear blocker, handling the message manager targets would be hard to track.
• then we have many options:
  • think about the integration between TargetConfiguration parent process actor and DevToolsFrameParent JSWindowActor. Probably come up with a framework API to clarify the connection between these two.
  • have TargetConfiguration listen to window-global-created/destroyed and completely uncouple it from the JSWindowActor pair. It would still need to shared getAllRemoteBrowsingContexts from the Watcher actor...
  • may be have actors implementation split over all target processes and thread, like CDP/RemoteAgent may significantly help handling such usecase. But that sounds like a more challenging/disruptive approach. Should we be able to have a BrowsingContextTargetActor equivalent instantiated in the Parent Process, which would receive the data entries as the existing actor class? This would help prevent putting TargetConfiguration specifics in middle of DevToolsFrameParent.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

unassigning myself as I'm working on blockers of this, and I'm not sure what would be left after

Comment 16

[Chris Peterson [:cpeterson]]

This bug is assigned to Fission Milestone M8, but has whiteboard tag dt-fission instead of dt-fission-m3-mvp.

I'm adding the dt-fission-m3-triage tag so DevTools Fission triage can decide whether this bug should be fixed for dt-fission-m3-mvp or can be postponed from Fission Milestone MVP to Future.

Comment 17

[Marco Mucci [:MarcoM]]

Removing triage whiteboard tag as meta bugs are not included as part of the project's MVP backlog. If there are individual dependencies which should be considered the whiteboard tag can be added to them.

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Marco Mucci [:MarcoM] from comment #17)

Removing triage whiteboard tag as meta bugs are not included as part of the project's MVP backlog. If there are individual dependencies which should be considered the whiteboard tag can be added to them.

Thanks. In that case, I will move this meta bug from Fission Milestone M8 to Future.

This meta bug has one blocking bug that is still open (bug 1698891) and it is not tracked for dt-fission-m3-mvp.