Making routing ids typesafe

[Charles Harrison]

Hi site-isolation folks,

I may be working to help rdsmith@ with moving RDH away from render view ids 

https://code.google.com/p/chromium/issues/detail?id=472869

He mentioned that previous people considered a typedef or something similar on RenderFrameID / RenderProcessID to make this type safe. Has anyone tried this? Should I attempt to do this in the refactoring?

I was planning on updating exposed methods to take alternate parameters (e.g. with the new types), to allow a slower migration to the new API.

[Charlie Reis]

[+lukasza, jam, rdsmith]

Thanks Charles!  We're excited to have your help here.

I know Łukasz has been hopeful for the typesafe ID idea as well.  We brought it up with John years ago, and I think he had some concerns about the cognitive overhead of adding these types to the public API.

John, can you weigh in here?  It does seem like it would be really easy to end up with nasty bugs migrating between render view IDs and render frame IDs.

Charlie

[Łukasz Anforowicz]

There is an in-review attempt to introduce a type-safe id type at crrev.com/1492413002.  And a pending CL for using it for SavePackageId (at crrev.com/1492283004).

I don't have and for now don't want to have an opinion on whether such type-safe id type is useful for routing ids or process ids (as these things are broadly used and therefore somewhat controversial).  I personally quite like the idea of having such type for SavePackageId and SaveItemId and reusing the type to implement SurfaceId, but then I am the author/proposer of base::IdType<...> so I am obviously biased :->

--

Thanks,

Lukasz

[Nasko Oskov]

John and I have discussed this in the past and the conclusion was that routing ids are used all over the codebase, so changing the type is going to be a lot of churn. However, we can allocate routing ids using a scheme and use the properties of this scheme to enforce typing. As far as I recall, Avi had a strawman CL out at some point, but not sure what happened to it.

As for other types of int ids, especially if they are more narrowly used, I'm all for making them typed.

[Charles Harrison]

I like the idea of allocating routing ids using a scheme! That's great middle ground, and could allow us to verify + type the IDs as they come into internal classes like ResourceDispatcherHost. We already sort of do this with GlobalRequestID, but with schemes and verification code in GlobalRequestID, we get real type safety.

[Avi Drissman]

My strawman CL was really a message typed into email, https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/_vSX2Ktf2_g/12lhrLDrEwAJ . I'm a huge fan of Łukasz's id-type proposal, and would recommend it over mine; mine is really only useful if we don't want to try to boil the ocean and switch over all the places we use int. If you're going from scratch, you should do it right.

[Łukasz Anforowicz]

<+danakj@ to CC>

--

Thanks,

Lukasz

[Nick Carter]

I support the allocation-scheme approach. And I like the type-safe wrapper approach, at least in theory. We can actually do both, and they would complement each other: when you go from int->wrapper, it's a natural place to enforce the allocation scheme.

For the wrapper types to be worth it, we must come up with something simple enough to be easily understood, but comprehensive enough to express all the weird things that happen with routing_ids and process_ids in the code today. Some open questions I have:

 - Do we use different wrapper types on the browser/ and renderer/ side (e.g. keyed to RenderFrame vs. RenderFrameHost)? What about in common/?

 - Will it be possible to use wrapper types in IPC messages? The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

 - If we use wrapper types in IPC messages, what about messages that need to be handled by more than one type of object? How much IPC magic will we need to add for that?

 - Will we be able to define a FrameOrProxyRouteID wrapper type, that can hold a value of more than one flavor, for cases like this? If so, is it still just an int wrapper, or is it some kind of tagged union?

 - For classes like MessageRouter, that accept all kinds of routing ids, can we meaningfully switch to type-safe wrappers?

 - Will we support wrappers for different child_process_id types as well (renderer, extension, nacl, utility), or is that a non-goal?

 - Do we expose a routing pair <process_id, route_id> pattern as well, or let people keep rolling their own as needed?

Also, I would suggest not working in base/ to begin with; we'll have more flexibility if we incubate the idea in content/, and hoist it out for general use once it's proven itself there. We don't have a solid case for base/ inclusion yet.

 - nick

[Charles Harrison]

These are all hard problems and I'm not sure I'm experienced enough to answer.

However, most of these are not blockers for the allocation-scheme approach. I think we could implement that today, and  build type support around it later (or not). In the meantime, we could sprinkle runtime sanity-checks over places in the code that should be rock solid.

[Łukasz Anforowicz]

On Fri, Dec 11, 2015 at 1:15 PM, Nick Carter <nca...@google.com> wrote:

I support the allocation-scheme approach. And I like the type-safe wrapper approach, at least in theory. We can actually do both, and they would complement each other: when you go from int->wrapper, it's a natural place to enforce the allocation scheme.

For the wrapper types to be worth it, we must come up with something simple enough to be easily understood, but comprehensive enough to express all the weird things that happen with routing_ids and process_ids in the code today.

I am not sure I agree.  It would be great if we had some way of expressing relationships between different id types (i.e. render_frame_proxy_routing_id VS render_frame_routing_id VS render_view_routing_id), but I believe that a simple, separate id type that has not relationship to other id types is beneficial on its own.

 

Some open questions I have:

 - Do we use different wrapper types on the browser/ and renderer/ side (e.g. keyed to RenderFrame vs. RenderFrameHost)? What about in common/?

 - Will it be possible to use wrapper types in IPC messages?

Yes - it would be possible to use the wrapper type in *payload* of IPC messages - see https://codereview.chromium.org/1496103002/patch/1/10032.

 

The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

That is a good question.  OTOH, does not knowing the answer for routing ids mean that we can't have base::IdType-based SavePackageId?

 

 - If we use wrapper types in IPC messages, what about messages that need to be handled by more than one type of object? How much IPC magic will we need to add for that?

 - Will we be able to define a FrameOrProxyRouteID wrapper type, that can hold a value of more than one flavor, for cases like this? If so, is it still just an int wrapper, or is it some kind of tagged union?

 - For classes like MessageRouter, that accept all kinds of routing ids, can we meaningfully switch to type-safe wrappers? 

 - Will we support wrappers for different child_process_id types as well (renderer, extension, nacl, utility), or is that a non-goal?

I think that for cases like above we can only rely on runtime checks to see if a generic routing_id is of a specific/narrower flavor (i.e. what you are asking for is like a dynamic_cast<...> from a base class to a specific class and this cannot be statically verified).

 

 - Do we expose a routing pair <process_id, route_id> pattern as well, or let people keep rolling their own as needed?

Maybe.  Does that decision depend on whether process_id is a raw int vs a separate id type? 

Also, I would suggest not working in base/ to begin with; we'll have more flexibility if we incubate the idea in content/, and hoist it out for general use once it's proven itself there. We don't have a solid case for base/ inclusion yet.

To support pickling into / out-of IPC message payload (required by cc::SurfaceId), we can do the following if base::IdType is in base: https://codereview.chromium.org/1496103002/patch/1/10032

We wouldn't be able to do this if base::IdType were in //content (because of no //ipc -> //content DEPS allowed). 

--

Thanks,

Lukasz

[Nick Carter]

On Fri, Dec 11, 2015 at 3:41 PM, Łukasz Anforowicz <luk...@google.com> wrote:

On Fri, Dec 11, 2015 at 1:15 PM, Nick Carter <nca...@google.com> wrote:

I support the allocation-scheme approach. And I like the type-safe wrapper approach, at least in theory. We can actually do both, and they would complement each other: when you go from int->wrapper, it's a natural place to enforce the allocation scheme.

For the wrapper types to be worth it, we must come up with something simple enough to be easily understood, but comprehensive enough to express all the weird things that happen with routing_ids and process_ids in the code today.

I am not sure I agree.  It would be great if we had some way of expressing relationships between different id types (i.e. render_frame_proxy_routing_id VS render_frame_routing_id VS render_view_routing_id), but I believe that a simple, separate id type that has not relationship to other id types is beneficial on its own. 

Some open questions I have:

 - Do we use different wrapper types on the browser/ and renderer/ side (e.g. keyed to RenderFrame vs. RenderFrameHost)? What about in common/?

 - Will it be possible to use wrapper types in IPC messages?

Yes - it would be possible to use the wrapper type in *payload* of IPC messages - see https://codereview.chromium.org/1496103002/patch/1/10032.

 

The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

That is a good question.  OTOH, does not knowing the answer for routing ids mean that we can't have base::IdType-based SavePackageId?

I see your point, but this only belongs in base if it's generally useful. I think routing ids are a good litmus test for that. If it doesn't work for routing_ids, I'm not necessary opposed to it: I just care a lot less about it.

 

 - If we use wrapper types in IPC messages, what about messages that need to be handled by more than one type of object? How much IPC magic will we need to add for that?

 - Will we be able to define a FrameOrProxyRouteID wrapper type, that can hold a value of more than one flavor, for cases like this? If so, is it still just an int wrapper, or is it some kind of tagged union?

 - For classes like MessageRouter, that accept all kinds of routing ids, can we meaningfully switch to type-safe wrappers? 

 - Will we support wrappers for different child_process_id types as well (renderer, extension, nacl, utility), or is that a non-goal?

I think that for cases like above we can only rely on runtime checks to see if a generic routing_id is of a specific/narrower flavor (i.e. what you are asking for is like a dynamic_cast<...> from a base class to a specific class and this cannot be statically verified).

Are you suggesting we would leave them as ints, then?

  - Do we expose a routing pair <process_id, route_id> pattern as well, or let people keep rolling their own as needed?

Maybe.  Does that decision depend on whether process_id is a raw int vs a separate id type? 

There's not a hard dependency, but if we're touching all routing ids anyway, it makes sense to consider unifying this too. I was only asking whether we should.

 

Also, I would suggest not working in base/ to begin with; we'll have more flexibility if we incubate the idea in content/, and hoist it out for general use once it's proven itself there. We don't have a solid case for base/ inclusion yet.

To support pickling into / out-of IPC message payload (required by cc::SurfaceId), we can do the following if base::IdType is in base: https://codereview.chromium.org/1496103002/patch/1/10032

How does pickling work if TypeBeingIdentified only exists on one side of the IPC pipe (e.g. it's a browser-only object like RenderFrameHost)?

 

We wouldn't be able to do this if base::IdType were in //content (because of no //ipc -> //content DEPS allowed). 

I missed why we are even targeting cc::SurfaceId initially? It already has some level of type safety. Moreover, you can define ParamTraits anywhere, not just in //ipc. E.g. content/common/content_param_traits.h

[Antoine Labour]

On Sat, Dec 12, 2015 at 9:40 AM, 'Nick Carter' via Site Isolation Development <site-isol...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 3:41 PM, Łukasz Anforowicz <luk...@google.com> wrote:

On Fri, Dec 11, 2015 at 1:15 PM, Nick Carter <nca...@google.com> wrote:

I support the allocation-scheme approach. And I like the type-safe wrapper approach, at least in theory. We can actually do both, and they would complement each other: when you go from int->wrapper, it's a natural place to enforce the allocation scheme.

For the wrapper types to be worth it, we must come up with something simple enough to be easily understood, but comprehensive enough to express all the weird things that happen with routing_ids and process_ids in the code today.

I am not sure I agree.  It would be great if we had some way of expressing relationships between different id types (i.e. render_frame_proxy_routing_id VS render_frame_routing_id VS render_view_routing_id), but I believe that a simple, separate id type that has not relationship to other id types is beneficial on its own.  

Some open questions I have:

 - Do we use different wrapper types on the browser/ and renderer/ side (e.g. keyed to RenderFrame vs. RenderFrameHost)? What about in common/?

 - Will it be possible to use wrapper types in IPC messages?

Yes - it would be possible to use the wrapper type in *payload* of IPC messages - see https://codereview.chromium.org/1496103002/patch/1/10032.

 

The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

That is a good question.  OTOH, does not knowing the answer for routing ids mean that we can't have base::IdType-based SavePackageId?

I see your point, but this only belongs in base if it's generally useful. I think routing ids are a good litmus test for that. If it doesn't work for routing_ids, I'm not necessary opposed to it: I just care a lot less about it.

I'm a huge supporter of TypeId.

I think the solution allows for incremental use of type-safe ids. I.e. they can be wrapped/unwrapped as needed on API boundaries.

I think it's tremendously useful for routing ids - especially for routing ids actually - we can adopt them in many places without necessarily changing APIs (at the cost of locally wrapping/unwrapping), and progressively adding type safety in APIs, removing the wrapping/unwrapping churn. I.e. no need to boil the ocean :)

 

 

 - If we use wrapper types in IPC messages, what about messages that need to be handled by more than one type of object? How much IPC magic will we need to add for that?

 - Will we be able to define a FrameOrProxyRouteID wrapper type, that can hold a value of more than one flavor, for cases like this? If so, is it still just an int wrapper, or is it some kind of tagged union?

 - For classes like MessageRouter, that accept all kinds of routing ids, can we meaningfully switch to type-safe wrappers? 

 - Will we support wrappers for different child_process_id types as well (renderer, extension, nacl, utility), or is that a non-goal?

I think that for cases like above we can only rely on runtime checks to see if a generic routing_id is of a specific/narrower flavor (i.e. what you are asking for is like a dynamic_cast<...> from a base class to a specific class and this cannot be statically verified).

Are you suggesting we would leave them as ints, then?

  - Do we expose a routing pair <process_id, route_id> pattern as well, or let people keep rolling their own as needed?

Maybe.  Does that decision depend on whether process_id is a raw int vs a separate id type? 

There's not a hard dependency, but if we're touching all routing ids anyway, it makes sense to consider unifying this too. I was only asking whether we should.

 

Also, I would suggest not working in base/ to begin with; we'll have more flexibility if we incubate the idea in content/, and hoist it out for general use once it's proven itself there. We don't have a solid case for base/ inclusion yet.

To support pickling into / out-of IPC message payload (required by cc::SurfaceId), we can do the following if base::IdType is in base: https://codereview.chromium.org/1496103002/patch/1/10032

How does pickling work if TypeBeingIdentified only exists on one side of the IPC pipe (e.g. it's a browser-only object like RenderFrameHost)?

TypeBeingIdentified is just a type discriminator - it's not used for anything, it doesn't even need to be complete - just forward declared. We could just define an empty RenderFrameIdTypeDiscriminator struct in content/common if using RenderFrame or RenderFrameHost is somehow controversial.

 

 

We wouldn't be able to do this if base::IdType were in //content (because of no //ipc -> //content DEPS allowed). 

I missed why we are even targeting cc::SurfaceId initially? It already has some level of type safety.

IdType is a generalization of SurfaceId. So it makes perfect sense to get SurfaceId to use IdType, if only to prove to ourselves that we didn't lose anything. 

 

Moreover, you can define ParamTraits anywhere, not just in //ipc. E.g. content/common/content_param_traits.h

If IdType is defined in a place visible by ipc (i.e. pretty much base/ or ipc/), then a single template ParamTraits definition is needed for all instances of IdType.

I guess we can either put IdType in base, or copy-pasta all over the place.

I'm not sure why there is this much opposition. Type safety proved useful for SurfaceId. There's evidence that routing ids would benefit from it as well. The proposed solution is incremental, has no run-time cost. In the end game, the complexity cost is virtually nil except at the id generation locus. Take a look at the SavePackageId CL for an excellent example - it doesn't change any code except using the type-safe type everywhere instead of the raw int.

Antoine

[Nick Carter]

On Mon, Dec 14, 2015 at 12:18 AM, Antoine Labour <pi...@chromium.org> wrote: 

The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

That is a good question.  OTOH, does not knowing the answer for routing ids mean that we can't have base::IdType-based SavePackageId?

I see your point, but this only belongs in base if it's generally useful. I think routing ids are a good litmus test for that. If it doesn't work for routing_ids, I'm not necessary opposed to it: I just care a lot less about it.

I'm a huge supporter of TypeId.

I think the solution allows for incremental use of type-safe ids. I.e. they can be wrapped/unwrapped as needed on API boundaries.

I think it's tremendously useful for routing ids - especially for routing ids actually - we can adopt them in many places without necessarily changing APIs (at the cost of locally wrapping/unwrapping), and progressively adding type safety in APIs, removing the wrapping/unwrapping churn. I.e. no need to boil the ocean :)

I, too, am strongly in favor of some kind of IdType for routing ids -- and I'm sorry if it came across differently! My questions were meant as questions, not arguments against. The currently proposed IdType doesn't seem to work for routing IDs end-to-end, so I'm trying to understand which shortcomings are inherent, and which are ones that we'll be able to iteratively improve.

For example, creating a FrameOrProxyRoutingId or an generic-flavor RoutingId wrapper seems totally tractable, though I don't quite see how to use the proposed template for that. Making IPC message ctors accept IdType instead of int seems implausibly tough. One big gotcha is any case like FrameMsg_UpdateOpener, which is sent to both frames and proxies, so you'd either need the message ctor to accept a FrameOrProxyRoutingId argument, or duplicate the message definition.

It's starting to look as if "int routing_id" is too baked into IPC::Message to be worth changing, unless somebody has a clever way to do it.

 

How does pickling work if TypeBeingIdentified only exists on one side of the IPC pipe (e.g. it's a browser-only object like RenderFrameHost)?

TypeBeingIdentified is just a type discriminator - it's not used for anything, it doesn't even need to be complete - just forward declared. We could just define an empty RenderFrameIdTypeDiscriminator struct in content/common if using RenderFrame or RenderFrameHost is somehow controversial.

If that's the choice, the content/common approach is what I would advocate. Use of renderer-only class types in browser/ runs very against convention, and looks wrong even if it's just a forward declaration.

 

Moreover, you can define ParamTraits anywhere, not just in //ipc. E.g. content/common/content_param_traits.h

If IdType is defined in a place visible by ipc (i.e. pretty much base/ or ipc/), then a single template ParamTraits definition is needed for all instances of IdType.

I guess we can either put IdType in base, or copy-pasta all over the place.

These arguments are convincing, and I understand now why /base makes sense for this.

I'm not sure why there is this much opposition. Type safety proved useful for SurfaceId. There's evidence that routing ids would benefit from it as well. The proposed solution is incremental, has no run-time cost. In the end game, the complexity cost is virtually nil except at the id generation locus. Take a look at the SavePackageId CL for an excellent example - it doesn't change any code except using the type-safe type everywhere instead of the raw int.

I agree, SavePackageId is enhanced by using the proposed TypeId, and it looks like SavePackageId is pretty similar to SurfaceId. Routing IDs (the topic of this thread), encounter complications not seen by SavePackageId.

 - nick

 

[Antoine Labour]

On Tue, Dec 15, 2015 at 6:26 AM, Nick Carter <nca...@google.com> wrote:

On Mon, Dec 14, 2015 at 12:18 AM, Antoine Labour <pi...@chromium.org> wrote: 

The ctor of a IPC::Message classes is the place where we most commonly use routing ids, so it would be natural to add safety there. Can we do this without touching every IPC message declaration? How does it work?

That is a good question.  OTOH, does not knowing the answer for routing ids mean that we can't have base::IdType-based SavePackageId?

I see your point, but this only belongs in base if it's generally useful. I think routing ids are a good litmus test for that. If it doesn't work for routing_ids, I'm not necessary opposed to it: I just care a lot less about it.

I'm a huge supporter of TypeId.

I think the solution allows for incremental use of type-safe ids. I.e. they can be wrapped/unwrapped as needed on API boundaries.

I think it's tremendously useful for routing ids - especially for routing ids actually - we can adopt them in many places without necessarily changing APIs (at the cost of locally wrapping/unwrapping), and progressively adding type safety in APIs, removing the wrapping/unwrapping churn. I.e. no need to boil the ocean :)

I, too, am strongly in favor of some kind of IdType for routing ids -- and I'm sorry if it came across differently! My questions were meant as questions, not arguments against. The currently proposed IdType doesn't seem to work for routing IDs end-to-end, so I'm trying to understand which shortcomings are inherent, and which are ones that we'll be able to iteratively improve.

For example, creating a FrameOrProxyRoutingId or an generic-flavor RoutingId wrapper seems totally tractable, though I don't quite see how to use the proposed template for that. Making IPC message ctors accept IdType instead of int seems implausibly tough. One big gotcha is any case like FrameMsg_UpdateOpener, which is sent to both frames and proxies, so you'd either need the message ctor to accept a FrameOrProxyRoutingId argument, or duplicate the message definition.

It's starting to look as if "int routing_id" is too baked into IPC::Message to be worth changing, unless somebody has a clever way to do it.

Are you specifically thinking about the first implicit parameter of IPC_MESSAGE_ROUTED*?

Is it just a matter of adding IPC_MESSAGE_ROUTED_TYPED* macros that lets you specify the type of the implicit parameter? The constructor would take the typed id as a parameter (providing type safety) and then unwrap.

If you want to forego type safety it's very possible to add a template constructor to the message types generated by IPC_MESSAGE_ROUTED* that takes an IdType<T> and just unwraps - saving you the explicit unwrap at the call site. Then incrementally you can move them to IPC_MESSAGE_ROUTED_TYPED*

Even if there's a couple of messages that can't take full advantage of type safety, I don't think it blocks the many different places where we can make use of it. Even for those few places where we don't have a perfect solution, we can still move the unwrapping to close to where we need raw ids, so that we keep static typing in the rest of the code.

Re: FrameOrProxyRoutingId, it's possible to create the type as a direct typedef of IdType<FrameOrProxyTag>, or even make it derived from it to add a couple of constructors that take an IdType<FrameTag> and an IdType<FrameProxyTag> respectively, to add the implicit conversion to the more generic type. It's an edge case, and a specialized solution is appropriate for that.

Antoine

[Łukasz]

I've discussed this with Nick on Tuesday and:

• For now I'll try to repackage things into a CL that 1) adds id_type.h to //content rather than //base and 2) uses it to implement SavePackageId and SaveItemId.  See: crrev.com/1529363006.
• Pros
• This seems less controversial than //base changes and consequently has a higher chance of landing.
• This will allow further experimentation with other id-types under //content
• This takes care of SavePackageId and SaveItemId which for me were the main motivation for going in this direction.
• Cons
• For now SurfaceId CL will be abandoned / SurfaceId will remain a separate implementation.  There is a small risk that content::IdType<...> will evolve in a way that makes it difficult to adopt it in the future by SurfaceId
• Neutral
• There are some things in content::IdType<...> that are not needed by SavePackageId and/or SaveItemId (i.e. GetUnsafeValue, some map kinds), but 1) AFAIK unused template parts should be compiled out and 2) the code has reasonable unit tests coverage so shouldn't rot.

• For some scenarios, a hierarchy of id types might be desirable - for example: FrameRoutingId --is-a--> FrameOrFrameProxyRoutingId --is-a--> AnyRoutingId.
• I did some experimentation on Wednesday and classes derived from IdType<...> are possible, but are rather clunky around std::hash<T> (requiring each and every derived class to duplicate std::hash definition / partial specialization).  I am guessing that we would hit similar clunkiness around ipc::ParamTraits<T>.  OTOH, I did add some extra derived-class tests to id_type_unittest.cc.
• I think that even without a hierarchy, a single RoutingId type would still provide extra value (by making it distinct at compile-time from SavePackageId, SaveItemId and ProcessId [this is not just hypothetical :-) - all 4 kinds are used as parameters of SaveFileManager::OnSaveURL])

• In some scenarios it might be difficult to migrate to IdType-based approach in one shot.  In such scenarios it might be desirable to temporarily allow implicit coercions (i.e. if GetRoutingId returns a strongly-typed IdType, then it might be nice if code passing the return value to Message constructors doesn't have to change and explicitly call GetUnsafeValue).  OTOH, in scenarios where full adoption of IdType-based approach is possible (i.e. SavePackageId), introducing implicit coercions is undesirable.  Therefore it seems to me that the decision to allow implicit coercions (both ways) should be left to individual id types and not included in the generic IdType<...> template.  I understand that this is far from ideal :-/ - it means either deriving those relaxed-case-id-types from IdType<...> template (but then see std::hash trouble in the previous bullet) or defining such relaxed-case-id-types from scratch, by copy&pasting IdType<...> code.

[Antoine Labour]

On Fri, Dec 18, 2015 at 7:17 AM, Łukasz <luk...@chromium.org> wrote:

I've discussed this with Nick on Tuesday and:

• For now I'll try to repackage things into a CL that 1) adds id_type.h to //content rather than //base and 2) uses it to implement SavePackageId and SaveItemId.  See: crrev.com/1529363006.
• Pros
• This seems less controversial than //base changes and consequently has a higher chance of landing.

Ugh, that's unfortunate. Besides cc, here's another example of a fix for a bug that would not have existed if we had an IdType: https://codereview.chromium.org/1525273003 . IdType would have been needed in gpu/

content, cc, gpu... Heck, if we want to make a RoutingId type being understood natively by IPCs, it needs to be visible in ipc/ as well.

There's a fundamental reason why this is useful in Chrome in general - we need different subsystems to refer to the same objects. Typically one uses pointers for that. However, when things like security boundaries, address spaces, VMs or even sometimes threads are involved, pointers aren't safe, and that's when we typically use ids + (hash)maps instead. This is pervasive in Chrome. Pointers express both identity and (static) type, so it is useful to add static typing to any id that effectively replaces a pointer.

IMO that clearly meets the bar for base/.

• This will allow further experimentation with other id-types under //content
• This takes care of SavePackageId and SaveItemId which for me were the main motivation for going in this direction.
• Cons
• For now SurfaceId CL will be abandoned / SurfaceId will remain a separate implementation.  There is a small risk that content::IdType<...> will evolve in a way that makes it difficult to adopt it in the future by SurfaceId
• Neutral
• There are some things in content::IdType<...> that are not needed by SavePackageId and/or SaveItemId (i.e. GetUnsafeValue, some map kinds), but 1) AFAIK unused template parts should be compiled out and 2) the code has reasonable unit tests coverage so shouldn't rot.

• For some scenarios, a hierarchy of id types might be desirable - for example: FrameRoutingId --is-a--> FrameOrFrameProxyRoutingId --is-a--> AnyRoutingId.
• I did some experimentation on Wednesday and classes derived from IdType<...> are possible, but are rather clunky around std::hash<T> (requiring each and every derived class to duplicate std::hash definition / partial specialization).  I am guessing that we would hit similar clunkiness around ipc::ParamTraits<T>.  OTOH, I did add some extra derived-class tests to id_type_unittest.cc.

Yeah, it sounds a bit annoying, requiring a 1-liner for each derived type to provide specializations (could be a macro). That said, we can probably improve ParamTraits to allow partial specializations with sfinae. 

• I think that even without a hierarchy, a single RoutingId type would still provide extra value (by making it distinct at compile-time from SavePackageId, SaveItemId and ProcessId [this is not just hypothetical :-) - all 4 kinds are used as parameters of SaveFileManager::OnSaveURL])

Maybe a generic RoutingId is useful, but beyond a few cases where we have type inheritance for those id (it's really an edge case), there's plenty more where having an explicit type in the message would be even better. 

• In some scenarios it might be difficult to migrate to IdType-based approach in one shot.  In such scenarios it might be desirable to temporarily allow implicit coercions (i.e. if GetRoutingId returns a strongly-typed IdType, then it might be nice if code passing the return value to Message constructors doesn't have to change and explicitly call GetUnsafeValue).  OTOH, in scenarios where full adoption of IdType-based approach is possible (i.e. SavePackageId), introducing implicit coercions is undesirable.  Therefore it seems to me that the decision to allow implicit coercions (both ways) should be left to individual id types and not included in the generic IdType<...> template.  I understand that this is far from ideal :-/ - it means either deriving those relaxed-case-id-types from IdType<...> template (but then see std::hash trouble in the previous bullet) or defining such relaxed-case-id-types from scratch, by copy&pasting IdType<...> code.

*if* we really want to do that, it can probably be done with an extra template argument, so that you don't need to introduce derived types. That said, there's probably a middle ground. For unwrapping, implicit conversion to the scalar type doesn't really seem dangerous, and that would take care of a bunch of use cases. For wrapping, an explicit constructor might be slightly more concise than FromUnsafeValue. 

Antoine

[Łukasz]

Dana, can you chime in here please?  Do you think that the progression of crrev.com/1529363006 -> crrev.com/1548443002 (and then possibly crrev.com/1496103002) makes sense?  What alternatives are there (dusting off crrev.com/1492413002?) / what other data (beyond what was provided in this thread) do we need before we pull the trigger?

Antoine, I tried to address your point inline, below.

Thanks,

Lukasz

On Thursday, December 17, 2015 at 10:21:11 PM UTC-8, Antoine Labour wrote:

On Fri, Dec 18, 2015 at 7:17 AM, Łukasz <luk...@chromium.org> wrote:

I've discussed this with Nick on Tuesday and:

• For now I'll try to repackage things into a CL that 1) adds id_type.h to //content rather than //base and 2) uses it to implement SavePackageId and SaveItemId.  See: crrev.com/1529363006.
• Pros
• This seems less controversial than //base changes and consequently has a higher chance of landing.

Ugh, that's unfortunate. Besides cc, here's another example of a fix for a bug that would not have existed if we had an IdType: https://codereview.chromium.org/1525273003 . IdType would have been needed in gpu/

Thanks for pointing out this bug - this is useful. 

content, cc, gpu... Heck, if we want to make a RoutingId type being understood natively by IPCs, it needs to be visible in ipc/ as well.

There's a fundamental reason why this is useful in Chrome in general - we need different subsystems to refer to the same objects. Typically one uses pointers for that. However, when things like security boundaries, address spaces, VMs or even sometimes threads are involved, pointers aren't safe, and that's when we typically use ids + (hash)maps instead. This is pervasive in Chrome. Pointers express both identity and (static) type, so it is useful to add static typing to any id that effectively replaces a pointer.

IMO that clearly meets the bar for base/.

I am not necessarily disagreeing :-).  OTOH, we need to start somewhere and we can always move it to //base later (at least IMO a CL to move id_type.h from //content to //base should not be that much hassle to author).  To try to prove this point, I've tried to put together a CL that introduces a gpu::CommandBufferId after moving id_type.h from //content to //base - crrev.com/1548443002.  Notes:

• The transition was more or less smooth.  The only minor clunkiness was the necessity to move definitions of gpu::SyncToken constructors out of the header file to avoid the "[chromium-style] Complex constructor has an inlined body" error that started to fire after changing the type of gpu::SyncToken::command_buffer_id_ from an integer to a "complex" gpu::CommandBufferId type.
• Similarily to SurfaceId transition (temporarily abandoned crrev.com/1496103002) gpu::CommandBufferId also required support for base::IdType<...> via ipc::ParamTraits in ipc/ipc_message_utils.h
  
• I personally really like the verbosity of GetUnsafeValue and FromUnsafeValue - it makes it easy to find the transition points by grepping (+ the name makes transition points stand out in code reviews).  For gpu::CommandBufferId the "unsafe" transitions are here:
• Defining some test constants (i.e. const CommandBufferId kBufferId1 = CommandBufferId::FromUnsafeValue(0x123);)
• Generating the next unique id value:
• gpu/command_buffer/tests/gl_manager.cc: 124: constructor of GLManager initializes command_buffer_id_ field.
  
• gpu/command_buffer/service/in_process_command_buffer.cc: 199: constructor of InProcessCommandBuffer initializes command_buffer_id_ field.
  
• Interop
• ipc/ipc_message_utils.h - definition of ipc::ParamTraits for base::IdType<...>
• gpu/ipc/gpu_command_buffer_traits.cc - definition of ipc::ParamTraits<gpu::SyncToken>::Log
  
• transition into and out of WaitSyncTokenCHROMIUM
• gpu/command_buffer/client/gles2_implementation.cc: 5644
  
• gpu/command_buffer/service/gles2_cmd_decoder.cc: 12604
  
• I am not familiar with gpu/ and am not able to evaluate whether ShareGroup tracing_guid should be kept as an integer VS as a CommandBufferId.  For now gpu/command_buffer/client/gles2_implementation.cc:152 constructs a ShareGroup using an unsafe CommandBufferId value