Separating routing ids

[Avi Drissman]

At the standup it was suggested that we talk over email about our proposals for separating routing ids.

My favorite is to move to 64-bit routing ids and then having lots of space for tag bits. Apparently that's not happening.

My next favorite is to keep a registry. Every valid routing id is "owned" by some object in the browser process. So we have something like:

class RoutingRegistry {  // singleton

 public:

  enum RoutingScope {

    kNone = -1,

    kRenderFrame,

    kRenderView,

    // etc

  };

  scoped_ptr<RoutingRegistration> AllocateRoutingRegistration(RoutingScope scope);

  // Gets the scope for the routing_id, or kNone if it is not a valid routing id.

  RoutingScope GetRoutingScope(int routing_id);

 private:

  // routing_id -> routing_scope

  map<int, int> active_routes;

};

class RoutingRegistration {

  ~RoutingRegistration() { // unregister the route in the RoutingRegistry }

}

No cap on the number of scopes, we can wrap safely now (the registry can check for re-use before re-issuing), and we can still keep passing ints around like we do today.

Thoughts?

Avi 

[Nasko Oskov]

This will all be internal to content/ and only to the places where we allocate routing ids and implementations of *::FromID()? 

My only concern is that we will be incurring an extra map lookup, but we can try it and see if it hurts us perf-wise.

[Avi Drissman]

Not just internal to content, but internal to content/browser. We're trying to solve the core problem of "what kind of routing id is this", right? This is literally a lookup table for that, but we can hide away the ramifications into the smallest space.

As for perf concerns, we totally can do

RenderFrameHost::FromID(id) {

  DCHECK_EQ(kRenderFrameHost, RoutingRegistry::GetRoutingScope(id))

  //...

}

etc, and only DCHECK it. I would like to run a few canaries with CHECK there, though, because our tests aren't necessarily comprehensive.

But this is my proposal.

(BTW, what other places besides *::From would we check the routing id's scope?)

Avi

[Nasko Oskov]

On Fri, Nov 13, 2015 at 11:28 AM, Avi Drissman <a...@chromium.org> wrote:

Not just internal to content, but internal to content/browser. We're trying to solve the core problem of "what kind of routing id is this", right? This is literally a lookup table for that, but we can hide away the ramifications into the smallest space.

As for perf concerns, we totally can do

RenderFrameHost::FromID(id) {

  DCHECK_EQ(kRenderFrameHost, RoutingRegistry::GetRoutingScope(id))

  //...

}

etc, and only DCHECK it. I would like to run a few canaries with CHECK there, though, because our tests aren't necessarily comprehensive.

I'm ok keeping it a CHECK for a while as long as perf dashboard doesn't scream at us.

 

But this is my proposal.

(BTW, what other places besides *::From would we check the routing id's scope?)

I don't think we care about other places, just wanted to ensure I'm understanding your proposal correctly and fully.

Overall, I like it! Especially since we don't need to partition the routing id space or try to grow it to int64, which was pretty substantial change when I tried it long time ago.

[Avi Drissman]

Thanks.

Now that I'm actually looking at things...

First, this wouldn't need to be global. Since routing ids are per-process, this can live on the RenderProcessHost.

Second, this fixes wrapping, so yay! Wrapping today is already broken, since MSG_ROUTING_CONTROL = INT32_MAX, so we need to wrap before we hit that.

Third, I'm completely talking about of my butt here. When I actually write code that I try to compile I'll be able to give real answers.

Avi

[Nick Carter]

I think the proposed interface (where we use RAII for route deregistration, using scoped_ptr) and the proposed mechanism of scope tracking (a std::map) are orthogonal: we could use the same RAII scheme, but have it be backed by a partitioning up of the space of integers.

Possible gotchas:

 - If GetRoutingScope is DCHECK only, do we still incur the tracking cost in release builds?

 - What if we want to do scope DCHECKS on other threads in the browser process? Do we need locks to access the map? If this is UI thread only, it's not all that different from 

 - What if we want to do scope DCHECKS in the renderer process? Do we track these associations in another map?

 - What if we want to look at a route_id in a crash report, and tell what kind of object it is? I think that's only easily possible with a stateless allocator.

 - Sometimes we wind up in possession of routing_ids whose objects have been deleted. Example: frame detach can delete descendent RFH's from arbitrary processes, and the browser process knows about that deletion prior to the renderer process. So we can have legal in-flight renderer->browser IPCs using these deleted routes. If this is a CONTROL type message or the routing_id is just a normal parameter, that makes a simple DCHECK_EQ(GetRoutingScope(id)) hard to use -- and this consideration means that we need to use a lot more care when adding those DCHECKS. Whereas, with a fixed partitioning scheme, we can still recover the type even after the object falls out of use, because the DCHECK doesn't depend on liveness.

 - enums and the content public API don't always get along. If the chrome/ needs to add its own flavors of routable object (does it?), we wind up with a pattern like process_type.h, which is gross. However, I think this is a problem we'd be likely to encounter with any RoutingRegistry-like scheme.

[Nick Carter]

On Fri, Nov 13, 2015 at 12:13 PM, Nick Carter <nca...@google.com> wrote:

I think the proposed interface (where we use RAII for route deregistration, using scoped_ptr) and the proposed mechanism of scope tracking (a std::map) are orthogonal: we could use the same RAII scheme, but have it be backed by a partitioning up of the space of integers.

Possible gotchas:

 - If GetRoutingScope is DCHECK only, do we still incur the tracking cost in release builds?

 - What if we want to do scope DCHECKS on other threads in the browser process? Do we need locks to access the map? If this is UI thread only, it's not all that different from 

To complete this thought: If this is UI thread only, it's not all that different from just doing DCHECK(RenderFrameHost::FromID(process_id, route_id) != nullptr) ?

[Avi Drissman]

I'm not sure what the problem we're trying to solve is, then. Someone was talking about partitioning routing ids, but now that I think about it, that doesn't seem to me to be the problem at all.

Right now we have a collection of message ids that are sent to one destination. We're trying to split the destination into two, and split the collection of message ids into two collections, each only being valid for one destination.

Is that the problem? What is the actual problem here?

[Nasko Oskov]

Originally it all started by reviewing a CL for extensions, where we expose FrameTreeNode ID and at some of the methods changed we now have 3 different IDs - RenderViewHost, RenderFrameHost, FrameTreeNode. The latter two had similar naming and are being used for looking up objects. Since FTN and RFH ids all start from 0, it is very possible to have FTN id be valid RFH id and incorrect lookup to be made if the two are mixed up.

This is what started the discussion - make it impossible for this to happen.

However, stepping back, the main problem is that FTN ids are not scoped to a process. We can solve the original concern by making RPH issued routing ids and FTN ids not colliding and that will likely be just fine.

[Antoine Labour]

On Fri, Nov 13, 2015 at 1:10 PM, Nasko Oskov <na...@chromium.org> wrote:

Originally it all started by reviewing a CL for extensions, where we expose FrameTreeNode ID and at some of the methods changed we now have 3 different IDs - RenderViewHost, RenderFrameHost, FrameTreeNode. The latter two had similar naming and are being used for looking up objects. Since FTN and RFH ids all start from 0, it is very possible to have FTN id be valid RFH id and incorrect lookup to be made if the two are mixed up.

This is what started the discussion - make it impossible for this to happen.

I may be missing some of the context, but it's fairly straightforward to add static typing to routing ids, by wrapping them into a (template?) class, without adding run-time overhead, that behaves like a pod. With a trivial hash function and operator<, they can still be put into hash_maps and maps or other containers, and it's also easy to keep types across IPCs. See e.g. cc/surfaces/surface_id.h where we do exactly that.

Antoine