Testing associated interfaces

[Lucas Gadani]

When we have an interface Foo, mojo bindings auto-generates a testing interceptor named FooInterceptorForTesting, which can be used to test mojo IPCs.

Unfortunately, when the interface is used through an AssociatedInterfacePtr<Foo>, the associated interface is hard-coded to use FooProxy directly, which implements final methods and can't be intercepted when testing. I understand the reasons this was done (https://chromium.googlesource.com/chromium/src/+/91165cea768b42e1852756f9c67a468cf8825181), but it makes it really hard to write code that tests IPCs that goes through associated interfaces.

Ideally, mojo should provide a way to test these interfaces. Perhaps it would be possible to override the MessageReceiverWithResponder in the proxy to intercept the messages? Or maybe there's a better solution?

[Ken Rockot]

On Wed, Jan 10, 2018 at 12:07 PM, Lucas Gadani <l...@chromium.org> wrote:

When we have an interface Foo, mojo bindings auto-generates a testing interceptor named FooInterceptorForTesting, which can be used to test mojo IPCs.

Unfortunately, when the interface is used through an AssociatedInterfacePtr<Foo>, the associated interface is hard-coded to use FooProxy directly, which implements final methods and can't be intercepted when testing. I understand the reasons this was done (https://chromium.googlesource.com/chromium/src/+/91165cea768b42e1852756f9c67a468cf8825181), but it makes it really hard to write code that tests IPCs that goes through associated interfaces.

To be perfectly clear, this is not the reason why you can't (shouldn't) use FooInterceptorForTesting with associated interfaces. Note that FooInterfacePtr also uses FooProxy.

The intended usage of InterceptorForTesting is to create a secondary pipe which you control. One end is bound to the IFT which by default forwards everything to another interface pipe. You can then selectively override individual methods if you want to do additional work beyond forwarding. This does not require modifying an existing InterfacePtr or replacing its FooProxy with something else.

You can even nominally apply this approach with associated interfaces, given that mojo::MakeRequestAssociatedWithDedicatedPipe will give you a new associated interface "pipe", but assuming your interface is associated for good reasons, this would be a bad thing to do in integration tests: we can no longer really guarantee FIFO with respect to other mutually associated interfaces, and so stuff would probably get flaky in the tests.

Ideally, mojo should provide a way to test these interfaces. Perhaps it would be possible to override the MessageReceiverWithResponder in the proxy to intercept the messages? Or maybe there's a better solution?

I think MessageReceiver is definitely wrong place to inject something like this. It deals with opaque Message objects, and there's no way you want to deserialize and reserialize them without using the generated bindings code. You really want to be either directly at the call site (i.e. pre-serialization) or directly at the receiving site (i.e. wedged between deserialization and dispatch). The use of final Proxy types makes this quite awkward at the call-site, so I would suggest we try to do something at receiving endpoints instead.

It might be relatively easy to wedge something into the Stub (sink) held by a Binding/AssociatedBinding, i.e. chain your own filtering mojom::Foo impl which forwards to the original sink.

+yzshen explicitly for his thoughts on this

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-mojo+unsubscribe@chromium.org.
To post to this group, send email to chromi...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CALQVofpLsNq67HQdMLEJ6W2XD6Wd%3DGPxaqcA8MYXs%2B5Q4UHE-w%40mail.gmail.com.

[Marijn Kruisselbrink]

So I'm currently trying to do something similar (or at least about to try) to test some security properties of the mojo-fied version of blob URLs (crbug.com/777585), where the interface I'm trying to intercept is a associated interface (BlobURLStore, associated to a BlobRegistry). 

Would it work to use FooInterceptorForTesting to intercept (in my case) the BlobRegistry (but more generically whatever non-associated interface the associated interface is associated with), overriding the method that would normally return the associated interface to instead return a interceptor that just wraps the real BlobURLStore? It seems like that should just work, and should maintain all he proper associated/ordering bits?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CA%2BapAgFfoUnva-gASyvkjde0A-0bx5FyHTfg%3D18a8O%2BFCtLYiA%40mail.gmail.com.

[Ken Rockot]

On Wed, Jan 10, 2018 at 1:52 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

So I'm currently trying to do something similar (or at least about to try) to test some security properties of the mojo-fied version of blob URLs (crbug.com/777585), where the interface I'm trying to intercept is a associated interface (BlobURLStore, associated to a BlobRegistry). 

Would it work to use FooInterceptorForTesting to intercept (in my case) the BlobRegistry (but more generically whatever non-associated interface the associated interface is associated with), overriding the method that would normally return the associated interface to instead return a interceptor that just wraps the real BlobURLStore? It seems like that should just work, and should maintain all he proper associated/ordering bits?

Oh right. Of course you can also have an InterceptorForTesting forward to a real impl rather than acting as a proxy between two pipes.

That should work for any type of interface. WDYT Lucas?

[Lucas Gadani]

On Wed, Jan 10, 2018 at 4:10 PM Ken Rockot <roc...@google.com> wrote:

On Wed, Jan 10, 2018 at 12:07 PM, Lucas Gadani <l...@chromium.org> wrote:

When we have an interface Foo, mojo bindings auto-generates a testing interceptor named FooInterceptorForTesting, which can be used to test mojo IPCs.

Unfortunately, when the interface is used through an AssociatedInterfacePtr<Foo>, the associated interface is hard-coded to use FooProxy directly, which implements final methods and can't be intercepted when testing. I understand the reasons this was done (https://chromium.googlesource.com/chromium/src/+/91165cea768b42e1852756f9c67a468cf8825181), but it makes it really hard to write code that tests IPCs that goes through associated interfaces.

To be perfectly clear, this is not the reason why you can't (shouldn't) use FooInterceptorForTesting with associated interfaces. Note that FooInterfacePtr also uses FooProxy.

The intended usage of InterceptorForTesting is to create a secondary pipe which you control. One end is bound to the IFT which by default forwards everything to another interface pipe. You can then selectively override individual methods if you want to do additional work beyond forwarding. This does not require modifying an existing InterfacePtr or replacing its FooProxy with something else.

You can even nominally apply this approach with associated interfaces, given that mojo::MakeRequestAssociatedWithDedicatedPipe will give you a new associated interface "pipe", but assuming your interface is associated for good reasons, this would be a bad thing to do in integration tests: we can no longer really guarantee FIFO with respect to other mutually associated interfaces, and so stuff would probably get flaky in the tests.

Fair enough, I guess I am trying to use the interceptors a bit beyond what they were designed for. However, it would be a lot easier if we could just replace the {Associated,}InterfacePtr's interface after it's been created with a forwarding interface, instead of adding hooks everywhere the interfaces are created, which means adding test hooks to production code.

Ideally, mojo should provide a way to test these interfaces. Perhaps it would be possible to override the MessageReceiverWithResponder in the proxy to intercept the messages? Or maybe there's a better solution?

I think MessageReceiver is definitely wrong place to inject something like this. It deals with opaque Message objects, and there's no way you want to deserialize and reserialize them without using the generated bindings code. You really want to be either directly at the call site (i.e. pre-serialization) or directly at the receiving site (i.e. wedged between deserialization and dispatch). The use of final Proxy types makes this quite awkward at the call-site, so I would suggest we try to do something at receiving endpoints instead.

The only reason I thought of this is because that's how Chrome IPC handles things -- an IPC::MessageFilter can be used in any opaque message, but I guess mojo handles things differently.

[Lucas Gadani]

On Wed, Jan 10, 2018 at 4:59 PM Ken Rockot <roc...@google.com> wrote:

On Wed, Jan 10, 2018 at 1:52 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

So I'm currently trying to do something similar (or at least about to try) to test some security properties of the mojo-fied version of blob URLs (crbug.com/777585), where the interface I'm trying to intercept is a associated interface (BlobURLStore, associated to a BlobRegistry). 

Would it work to use FooInterceptorForTesting to intercept (in my case) the BlobRegistry (but more generically whatever non-associated interface the associated interface is associated with), overriding the method that would normally return the associated interface to instead return a interceptor that just wraps the real BlobURLStore? It seems like that should just work, and should maintain all he proper associated/ordering bits?

Oh right. Of course you can also have an InterceptorForTesting forward to a real impl rather than acting as a proxy between two pipes.

That should work for any type of interface. WDYT Lucas?

It would kinda work. Since the impl lives on the renderer process, it makes a lot harder to write browser tests, since I'll need to implement IPCs back to tell the browser that things were OK.

[Ken Rockot]

Yeah, if the receiving endpoint is out-of-process then that is not a great approach IMHO. 

+reillyg@ - do you happen to have numbers for the code size reduction realized by devirtualizing mojom interface calls? I don't think we should be too concerned with the negligible runtime cost of the indirection, so unless it actually improved code size, maybe we should consider undoing that change?

In that case it would be pretty straightforward to wedge a filter in between an InterfacePtr and its pipe.

[Yuzhu Shen]

On Wed, Jan 10, 2018 at 1:10 PM, Ken Rockot <roc...@google.com> wrote:

On Wed, Jan 10, 2018 at 12:07 PM, Lucas Gadani <l...@chromium.org> wrote:

When we have an interface Foo, mojo bindings auto-generates a testing interceptor named FooInterceptorForTesting, which can be used to test mojo IPCs.

Unfortunately, when the interface is used through an AssociatedInterfacePtr<Foo>, the associated interface is hard-coded to use FooProxy directly, which implements final methods and can't be intercepted when testing. I understand the reasons this was done (https://chromium.googlesource.com/chromium/src/+/91165cea768b42e1852756f9c67a468cf8825181), but it makes it really hard to write code that tests IPCs that goes through associated interfaces.

To be perfectly clear, this is not the reason why you can't (shouldn't) use FooInterceptorForTesting with associated interfaces. Note that FooInterfacePtr also uses FooProxy.

The intended usage of InterceptorForTesting is to create a secondary pipe which you control. One end is bound to the IFT which by default forwards everything to another interface pipe. You can then selectively override individual methods if you want to do additional work beyond forwarding. This does not require modifying an existing InterfacePtr or replacing its FooProxy with something else.

You can even nominally apply this approach with associated interfaces, given that mojo::MakeRequestAssociatedWithDedicatedPipe will give you a new associated interface "pipe", but assuming your interface is associated for good reasons, this would be a bad thing to do in integration tests: we can no longer really guarantee FIFO with respect to other mutually associated interfaces, and so stuff would probably get flaky in the tests.

Ideally, mojo should provide a way to test these interfaces. Perhaps it would be possible to override the MessageReceiverWithResponder in the proxy to intercept the messages? Or maybe there's a better solution?

I think MessageReceiver is definitely wrong place to inject something like this. It deals with opaque Message objects, and there's no way you want to deserialize and reserialize them without using the generated bindings code. You really want to be either directly at the call site (i.e. pre-serialization) or directly at the receiving site (i.e. wedged between deserialization and dispatch). The use of final Proxy types makes this quite awkward at the call-site, so I would suggest we try to do something at receiving endpoints instead.

It might be relatively easy to wedge something into the Stub (sink) held by a Binding/AssociatedBinding, i.e. chain your own filtering mojom::Foo impl which forwards to the original sink.

+yzshen explicitly for his thoughts on this

Sorry for late reply!

I think "wedging something into the Stub held by Binding/AssociatedBinding" is exactly how InterceptorForTesting is supposed to be used.

I can see the value of supporting intercepting at the Ptr side for testing purpose. If there isn't a significant cost to re-virtualizing the mojom interface calls, it seems reasonable to me. I would wait for Reilly's comments on this.

[Reilly Grant]

Devirtualization had no code size impact because the change in each function call site is only a few bytes and gets lost in the noise of function alignment. There's an unknown impact to performance in general from reducing the number of indirect jumps. I'm somewhat uncertain what issue the current devirtualization strategy is having here. Is the goal to replace the Interface* inside an InterfacePtr with an interceptor and the fact that I changed get() to return Proxy* making that impossible?

[Ken Rockot]

On Wed, Jan 10, 2018 at 3:13 PM, Reilly Grant <rei...@chromium.org> wrote:

Devirtualization had no code size impact because the change in each function call site is only a few bytes and gets lost in the noise of function alignment. There's an unknown impact to performance in general from reducing the number of indirect jumps. I'm somewhat uncertain what issue the current devirtualization strategy is having here. Is the goal to replace the Interface* inside an InterfacePtr with an interceptor and the fact that I changed get() to return Proxy* making that impossible?

Yes. The idea is that in testing, you may want to hack up an InterfacePtr such that its get() returns a testing Foo* that does whatever it wants before forwarding to the Ptr's underlying FooProxy.

to result in a direct call to a FooInterceptorForTesting (which implements Foo) 

[Ken Rockot]

On Wed, Jan 10, 2018 at 3:16 PM, Ken Rockot <roc...@google.com> wrote:

On Wed, Jan 10, 2018 at 3:13 PM, Reilly Grant <rei...@chromium.org> wrote:

Devirtualization had no code size impact because the change in each function call site is only a few bytes and gets lost in the noise of function alignment. There's an unknown impact to performance in general from reducing the number of indirect jumps. I'm somewhat uncertain what issue the current devirtualization strategy is having here. Is the goal to replace the Interface* inside an InterfacePtr with an interceptor and the fact that I changed get() to return Proxy* making that impossible?

Yes. The idea is that in testing, you may want to hack up an InterfacePtr such that its get() returns a testing Foo* that does whatever it wants before forwarding to the Ptr's underlying FooProxy.

to result in a direct call to a FooInterceptorForTesting (which implements Foo) 

Mentally delete this line, please