--
You received this message because you are subscribed to the Google Groups "Blink Isolation discussions" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-isolation...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-isolation-dev/CAAKSqgV_ib3JAEaMBtn-uqwP1n0vAHvswc0AinEDszeNvrK%2BKQ%40mail.gmail.com.
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzTd7UwgYwucjZFd1%3DR%2B6vRP0JcJF6Hjm0vRFeMp57MeQ%40mail.gmail.com.
--
To unsubscribe from this group and stop receiving emails from it, send an email to site-isolation-...@chromium.org.
Hi all,
Thanks for the feedback. Following the replies here and some offline chats, I'd like to clarify some things.
First, a short overview of how BIB works:
On the renderer side, the code obtains a reference to a BrowserInterfaceBrokerProxy (for ExecutionContext-scoped BIB) or ThreadSafeBrowserInterfaceBrokerProxy (for process-wide BIB), and proceeds to call GetInterface with a pending receiver.
On the browser side, BrowserInterfaceBrokerImpl<HostType> receives the GetInterface call, and checks its binder maps to see whether there is a binder for the requested interface type. For this to work, the binder maps need to be populated. This is done by the various hosts (e.g. RenderFrameHostImpl) instantiating their BIB and calling internal::PopulateBinderMap{WithContext} (implemented in browser_interface_binders.cc). There, content:: code may register its binders, and then call the ContentBrowserClient to allow the embedder to register its own binders.
For this discussion, I am mostly concerned with the public API (for now). Because of this, I am mostly interested in this last call into the ContentBrowserClient, as it would have to pass the current context/scope to the client.
From an API perspective, I think the options are (with some pros and cons):
Expose the ASG concept to clients (either ASGH or some sort of token/ID)
[Pro] Easy to implement since we already have ASG/ASGH.
[Pro] No lifetime issues, since ASG lifetime is already "managed" by content.
[Con] The ASG boundary is somewhat arbitrary, making it harder for clients to reason about.
[Con] Would make it harder to introduce an Agent concept in the future (basically we would be encountering all the same questions and issues we are encountering now when trying to figure out if an interface can or can't be migrated to a frame's BIB).
Expose the Agent concept to clients (see implementation note below)
[Pro] Agent is a spec concept, that is clearly defined, and is the "natural" scope for some interfaces.
[Pro] Smaller scope means shorter lifetime of mojo interfaces and more fine-grained control of who is allowed to use them.
[Con] Introduction of new concepts to content, managing lifecycle, etc.
[Con] Additional cognitive overhead for Blink developers. (I would argue this is actually "necessary" and not something we want to hide from developers, since it is part of the spec.)
Keep the new BIB "internal" and don't expose it to clients. May still be either ASG- or Agent- scoped.
[Pro] Allows us to defer the decision of what, if anything, to expose publicly.
+ most of the above arguments, depending on binding scope.
A note about implementation:
If we wanted to introduce an Agent-scoped BIB, it doesn't necessarily mean we need an Agent/AgentHost (or something equivalent). We would, however, need a mechanism to trigger the BIB instantiation and binder registration whenever a new Agent is "created".
A possible implementation might look something like this:
The content client API could look like RegisterAgentInterfaces(AgentToken, BinderMapWithContext<AgentToken>).
Whenever a frame is created belonging to a new agent, call ASGH::OnNewAgent(AgentToken).
ASGH::OnNewAgent will instantiate a BIB, save it in a map keyed by the AgentToken, and trigger the interface registration flow.
(Optionally) When the Agent is "destroyed", clean up its BIB.
+jam@, CSA team,
I would especially like to hear your thoughts as content API owners.
More specifically, it seems likely that until we have an actual use-case of an embedder binding requiring this, we would probably choose to proceed with option 3 above, but we would still like to reach some consensus on what the right scope for the new BIB is (to avoid having to make too many changes if we do decide to expose it in the future).
Thanks,
Tal
Friendly ping on the topic.We plan on adding per-AgentSchedulingGroup BrowserInterfaceBroker for content/ internal users soon, so we'd appreciate your feedback on this and the direction talp@ outlined earlier.We are more than happy to GVC if you have questions or need clarification.Thanks,kouheiOn Thu, Jan 21, 2021 at 3:59 PM Tal Pressman <ta...@google.com> wrote:AFAICT, currently there are very few interfaces registered by embedders:
- ContentSettingsManager [chrome][weblayer]
- SpellCheckHost / SpellCheckPanelHost [chrome]
- MetricsService [chrome]
- JsChannelBindingProvider [chromecast]
I don't know if any of them would require agent granularity, maybe CSM?On Thu, Jan 21, 2021 at 2:29 PM Kentaro Hara <har...@chromium.org> wrote:Would it be possible to list mojo interfaces that will require the per-Agent / ASG BIB and see if they require the //content/public/ APIs?On Thu, Jan 21, 2021 at 12:58 PM Tal Pressman <ta...@google.com> wrote:Hi all,
Thanks for the feedback. Following the replies here and some offline chats, I'd like to clarify some things.
First, a short overview of how BIB works:
On the renderer side, the code obtains a reference to a BrowserInterfaceBrokerProxy (for ExecutionContext-scoped BIB) or ThreadSafeBrowserInterfaceBrokerProxy (for process-wide BIB), and proceeds to call GetInterface with a pending receiver.
On the browser side, BrowserInterfaceBrokerImpl<HostType> receives the GetInterface call, and checks its binder maps to see whether there is a binder for the requested interface type. For this to work, the binder maps need to be populated. This is done by the various hosts (e.g. RenderFrameHostImpl) instantiating their BIB and calling internal::PopulateBinderMap{WithContext} (implemented in browser_interface_binders.cc). There, content:: code may register its binders, and then call the ContentBrowserClient to allow the embedder to register its own binders.
For this discussion, I am mostly concerned with the public API (for now). Because of this, I am mostly interested in this last call into the ContentBrowserClient, as it would have to pass the current context/scope to the client.
From an API perspective, I think the options are (with some pros and cons):
Expose the ASG concept to clients (either ASGH or some sort of token/ID)
[Pro] Easy to implement since we already have ASG/ASGH.
[Pro] No lifetime issues, since ASG lifetime is already "managed" by content.
[Con] The ASG boundary is somewhat arbitrary, making it harder for clients to reason about.
[Con] Would make it harder to introduce an Agent concept in the future (basically we would be encountering all the same questions and issues we are encountering now when trying to figure out if an interface can or can't be migrated to a frame's BIB).
Expose the Agent concept to clients (see implementation note below)
[Pro] Agent is a spec concept, that is clearly defined, and is the "natural" scope for some interfaces.
[Pro] Smaller scope means shorter lifetime of mojo interfaces and more fine-grained control of who is allowed to use them.
[Con] Introduction of new concepts to content, managing lifecycle, etc.
[Con] Additional cognitive overhead for Blink developers. (I would argue this is actually "necessary" and not something we want to hide from developers, since it is part of the spec.)
Keep the new BIB "internal" and don't expose it to clients. May still be either ASG- or Agent- scoped.
[Pro] Allows us to defer the decision of what, if anything, to expose publicly.
+ most of the above arguments, depending on binding scope.
A note about implementation:
If we wanted to introduce an Agent-scoped BIB, it doesn't necessarily mean we need an Agent/AgentHost (or something equivalent). We would, however, need a mechanism to trigger the BIB instantiation and binder registration whenever a new Agent is "created".
A possible implementation might look something like this:
The content client API could look like RegisterAgentInterfaces(AgentToken, BinderMapWithContext<AgentToken>).
Whenever a frame is created belonging to a new agent, call ASGH::OnNewAgent(AgentToken).
ASGH::OnNewAgent will instantiate a BIB, save it in a map keyed by the AgentToken, and trigger the interface registration flow.
(Optionally) When the Agent is "destroyed", clean up its BIB.
AFAICT, currently there are very few interfaces registered by embedders:
- SpellCheckHost / SpellCheckPanelHost [chrome]
- MetricsService [chrome]
I don't know if any of them would require agent granularity, maybe CSM?
- JsChannelBindingProvider [chromecast]
I want to clarify that ThreadSafeBrowserInterfaceBrokerProxy was a hack from the past that covered Mojo interfaces for workers as well. Since workers now have a per-ExecutionContext BrowserInterfaceBroker, once we no longer have per-process interfaces, we won't need the thread-safe BrowserInterfaceBroker.
I personally find these names a bit confusing and unintuitive--especially the latter
[...] for consistency, I would recommend resetting the message pipe endpoint and leaving it at that
Thanks, Daniel!I want to clarify that ThreadSafeBrowserInterfaceBrokerProxy was a hack from the past that covered Mojo interfaces for workers as well. Since workers now have a per-ExecutionContext BrowserInterfaceBroker, once we no longer have per-process interfaces, we won't need the thread-safe BrowserInterfaceBroker.I don't know whether we could actually replace all the interfaces, but if we can that would definitely be a very nice cleanup :)I personally find these names a bit confusing and unintuitive--especially the latterYeah, the names are pretty confusing... One note about your analogy, though, is that AgentGroupScheduler is actually analogous to FrameSchedulerImpl, not LocalFrame. We don't currently have an equivalent for LocalFrame, but would likely want to add one as part of adding the new BIB.[...] for consistency, I would recommend resetting the message pipe endpoint and leaving it at thatThe big issue here is, since we don't actually have an Agent object, knowing when it is destroyed. We could maybe keep track of the frames associated with the ASG, but I'm not sure if that's a good design...
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPVAxLWvbGOH2RuFrCpARs3_3j_4ZwATycTa%3Drzq2E3hYx148Q%40mail.gmail.com.
I vaguely wonder if we also want to move more ExecutionContext-specific things to an Agent level to avoid the flakiness concern above
It sounds an awful lot like document 1 could set up some browser-side feature state via Mojo, immediately tell same-origin document 2 to use that state, and document 2's feature-use IPC could arrive before document 1's feature-setup IPC and thus fails. I chatted with Daniel about it just now and it seems like people are mostly crossing their fingers that this general approach in Mojo is safe enough. I suppose the DOM storage APIs wouldn't face that problem since they're currently process-wide, but we have a ton of things in frame-level BiBs that seem like they could be flaky for closely interacting documents.
On the Blink side, most Mojo interfaces are ExecutionContext-specific and their connections are expected to reset when the ExecutionContext is detached. This is exactly why we migrated mojo::Remote/Receiver to HeapRemote/Receiver, whose default behavior is to reset the mojo connection when the ExecutionContext is detached.
vaguely wonder if we also want to move more ExecutionContext-specific things to an Agent level to avoid the flakiness concern above
I think this wouldn't be needed if BrowserInterfaceBroker was channel-associated *and* BrowserInterfaceBroker provided a mechanism for binding associated interfaces. But I would have some concerns about how this would interact with Blink scheduling, so hopefully someone can shed some light on that.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-isolation-dev/CAF3XrKrXE-yF6GWxB2c7EuSMGb1YMKK4C68VbCpLQwHzZt1RKg%40mail.gmail.com.
Just to clarify, are you proposing the end state like this?
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAKSqgVsp8%3DnitrUkfsEsnTPw7ZDTghL7uyxJWowYSmkGUTNNA%40mail.gmail.com.