New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 619207 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Ensure that mojo interfaces associated with ChannelMojo never block chrome IPC dispatching on the IO thread

Project Member Reported by jam@chromium.org, Jun 10 2016

Issue description

From https://docs.google.com/document/d/1iN5cz-zCqN09eWxk6w84ioGifOO4f7PMS33S-cYi-zw/edit and  bug 609607 , it came up that there's a scenario which would pause dispatching of chrome IPCs on the IO thread:
-if a mojo interface is associated with ChannelMojo and its interface request is processed on the UI thread (any thread on the browser other than the IO thread), then until that request is bound we will pause dispatching of chrome IPCs on the IO thread.

We need to programatically ensure this doesn't happen, or else performance will suffer. One of the main reasons for having an IO thread is that networking requests can occur without waiting on the UI thread.
 

Comment 1 by yzshen@chromium.org, Jun 10 2016

(To be exact, an unbound associated interface request itself doesn't block dispatching of messages. Message dispatching is paused when there is actually an incoming message targeting that unbound interface endpoint.)

In addition to preventing interface requests from being passed to other threads, we probably also want to ensure that users don't delay binding requests on the IO thread.

Comment 2 by roc...@chromium.org, Jun 10 2016

My current thinking is we could enforce that all interface requests are
bound synchronously during dispatch. This would be an opt-in constraint
enforced via DCHECK. I'd also like to add an AssociatedBindingProxy which
supports dispatch to a fixed TaskRunner which may run tasks a different
thread from the one the InterfaceEndpointClient lives on.

Given these features it should be possible for MultiplexRouter to drive
concurrent dispatch on multiple threads without any risk of ordering issues.

There is an additional constraint in that you couldn't have something like:

  interface Foo {  // Runs on IO thread, associated with Channel
    GetBar(associated Bar&);  // Binds to proxy on IO thread which
dispatches on UI thread
  };

  interface Bar {  // Runs on UI thread
    GetBaz(associated Baz&); // Binds to proxy on UI thread which
dispatches on IO thread (NOPE, BAD)
  };

This is bad because FIFO can't be guaranteed between Chrome IPC and Baz.

Still need to think more about this but WDYT?

Comment 3 by yzshen@chromium.org, Jun 11 2016

I would like to learn more details about the AssociatedBindingProxy next time we meet.

In the example of #2, if we enforce that all the associated interface requests should be bound synchronously during dispatch, then Foo, Bar, and Baz must be all on the IO thread, no?

Comment 4 by jam@chromium.org, Jun 13 2016

regarding "My current thinking is we could enforce that all interface requests are bound synchronously during dispatch. This would be an opt-in constraint enforced via DCHECK."

These two sentences seem mutually exclusive?

AFAIK we only need to enforce the former limitation (sync binding) just for interfaces that are associated with ChannelMojo. I don't know what the API is to do the association, but perhaps we can hide that so that only a wrapper method has access to the ChannelMojo object, and that one adds the constraint to the binding to enforce sync binding.

Comment 5 by roc...@chromium.org, Jun 13 2016

Re #4: What I mean is that you could elect at binding time to have your endpoint subjected to those extra constraints. With that option enabled, the router would DCHECK that all dispatched associated requests were bound synchronously. Without the option enabled the router would behave as it does today, i.e. allow async binding but not be sufficient for ChannelMojo's use case.

Re #3: Yes, you can't enforce sync binding unless either all bindings live on the same thread or we introduce some way of binding a request on a different thread from the task runner (known at binding time, and known by the MultiplexRouter) on which it dispatches messages. The second part of this proposal is to introduce the latter concept (call it "proxy-binding" for now.)

Re #4 again: In practice we may only need proxy-binding from IO-thread to UI-thread, so the latter limitation might not matter. Still something to be aware of.

Comment 6 by jam@chromium.org, Jun 14 2016

If this is an option to be be picked at runtime, the main thing I care about is that it's not possible for any random file in the code to modify their interface's selection. i.e. it should be enforced through a choke point that configures this correctly.
Components: Internals>Mojo

Comment 8 by roc...@chromium.org, May 29 2017

Status: Fixed (was: Available)
This was addressed a while ago when Channel-associated interfaces were actually implemented. The issue is avoided by using a custom replacement for MultiplexRouter which imposes extra restrictions with the trade-off that it never has to block any message dispatch.

Sign in to add a comment