I know nasko has been thinking about how to do message injection in mojo.

If we can get a pointer to the browser-process instance of the relevant mojo interface, I'd think it would suffice to just call methods on it directly -- in fact, that would be nicer, since you could actually capture the reply (which was never possible with PwnMessageReceived.

The other thing that PwnMessageReceived does, is a thread hop to the IO thread in all cases. That's likely not important for mojo security tests -- it mattered for legacy IPC because of the MessageFilter pattern, where objects on the IO thread could selectively filter and intercept certain messages that were destined for the UI thread. I think one of the first PwnMessageReceived tests we wrote was to test that a message was blocked by a particular IO thread message filter, so it needed that behavior.

> Of course it would be possible to merely verify that the received m1essage was considered a "Bad" message, without actually verifying that any particular renderer was killed (because without the association of a mojo message with a renderer process none would be killed).

I'm not sure I follow this part. We should retain the capacity to kill the remote renderer if it oversteps its security permissions. Is this tricky to accomplish with mojo blobs?

> I'm not sure I follow this part. We should retain the capacity to kill the remote renderer if it oversteps its security permissions. Is this tricky to accomplish with mojo blobs?

We do retain that capacity (and it works that way today). It's only that unless deep inside mojo (https://cs.chromium.org/chromium/src/mojo/edk/system/core.cc?type=cs&l=766) the source node of the message is a renderer process no process will be killed. So in this particular test scenario we'd only be able to detect the bad message by setting default_process_error_callback_, rather than by verifying that the renderer was killed.

Over chat Nasko pointed me to his two CLs. The first introduced the concept, the latter one improved on it:

https://chromium-review.googlesource.com/c/chromium/src/+/611102
https://chromium-review.googlesource.com/c/chromium/src/+/644187

Most of what follows is a paraphrasing of what he explained to me.

The idea is that the bindings generator generates an interceptor class. You can create an instance of it, override only the methods you care about. And then tell the bindings system to use that impl instead of the real one, installing it as a man-in-the-middle. And you need to implement one method - the one that forwards all non-overridden methods to an actual impl.

So, you have a real renderer talking to a real instance of the service, but your test has the ability to man-in-the-middle + mutate that message stream. This allows you to write tests similar to PwnMessageReceived, but it's not a drop-in-replacement, since it's a somewhat different primitive.

Nasko's approach is to have having real code drive the interactions, but modify the IPC the way you want it, instead of injecting a made up IPC message. It is somewhat closer to reality, and the tests should get a lot simpler and easier to maintain that way -- the blob security tests, for example, had to synthesize quite a bit of the conversation.

So I think the recipe would be something like this:
 - Write a more conventional browsertest (that creates a blob via javascript)
 - Install one of these interceptors, following the pattern in r644187 (looks like you call SwapImplForTesting).
 - Have the interceptor mutate the origin to be something illegal.
 - Wait for an actual kill, as before.

Does that sound viable here? I know nasko would welcome some feedback on this mechanism.

Yeah, that does sound viable. Problem for now is that initially mojo blobs doesn't quite replace blob URL registering yet (since that part is more complicated than the rest of it), so while the injecting-IPCs mechanism to create blobs in this test will no longer work after flipping the switch, replacing everything with a mojo based test won't quite work since the renderer would still be using old-style IPC for the actual URL registration... So I guess the main question is if I should try to write a test for the intermediate situation as well...

In theory, it seems like if you write an interceptor for the intermediate state whose purpose is to capture the blob ID, then you continue to do PwnMessageReceived() to do the public URL registration, then it should be a pretty small incremental change to enhance the interceptor once blob-URL registration moves over too. Right?

Alternatively -- you can just make content::PwnMessageHelper::CreateBlobWithPayload() work by any means necessary for now, and retool the test once URL registration gets ported to mojo. The CreateBlobWithPayload operation doesn't do any renderer kills, right (there's no origin involved)?.

I just looked at CreateBlobWithPayload again, and saw this step --

  // Block the renderer on operation that never completes, to shield it from
  // receiving unexpected browser->renderer IPCs that might CHECK.

That's a step that we ought to be able to do away with if we use nasko's interceptor framework, which is nice.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc

commit 604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Thu Oct 26 16:31:05 2017

Refactor ChromeSecurityExploitBrowserTest to work with mojo blobs.

This changes how this test creates blobs, so the tests work regardless
of the underlying blob IPC mechanism. Once blob URL management is also
migrated to mojo the tests for that will need to be rewritten to deal
with that, but this at least for now makes the test still meaningful
both with and without mojo blobs enabled.

Bug:  777585 
Change-Id: Ic1bfdfc11251dce85aee4ad74e4f90546e7df164
Reviewed-on: https://chromium-review.googlesource.com/736168
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511845}
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/browser/blob_storage/chrome_blob_storage_context.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/browser/blob_storage/chrome_blob_storage_context.h
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/browser/browser_context.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/browser/renderer_host/clipboard_message_filter.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/public/browser/browser_context.h
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/content/public/test/browser_test_utils.h
[modify] https://crrev.com/604fd7e78ca97c9a806d1dca0cfc03a4ad5a09cc/extensions/browser/api/printer_provider_internal/printer_provider_internal_api.cc

Apologies, applied the wrong component in bulk.

Setting Internals>Services>Storage to all children of issue 611935

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f1b1a702211eec197203a7bb0ef7039c7a2d61d

commit 8f1b1a702211eec197203a7bb0ef7039c7a2d61d
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Jan 26 18:09:56 2018

Refactor ChromeSecurityExploitBrowserTest to also work with mojo blob URLs.

Or to be more precise, add a version of the CreateBlobInExtensionOrigin
test that uses the mojo blob URL code path.

Bug:  777585 
Change-Id: Ife85fd3880994b2c2634dc7dbf54581ec0b7e30e
Reviewed-on: https://chromium-review.googlesource.com/882560
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532016}
[modify] https://crrev.com/8f1b1a702211eec197203a7bb0ef7039c7a2d61d/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/8f1b1a702211eec197203a7bb0ef7039c7a2d61d/mojo/public/cpp/bindings/strong_associated_binding.h
[modify] https://crrev.com/8f1b1a702211eec197203a7bb0ef7039c7a2d61d/storage/browser/blob/blob_registry_impl.cc
[modify] https://crrev.com/8f1b1a702211eec197203a7bb0ef7039c7a2d61d/storage/browser/blob/blob_registry_impl.h