Convert NaCl renderer-browser messages to mojo.

Convert NaCl renderer-browser messages to mojo.

BUG=577685

[Sam McNally, 2016-11-22 07:23:27]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 07:23:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 07:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 07:26:36]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2016-11-22 23:25:33]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:26:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 23:55:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:55:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sam McNally, 2016-11-23 00:08:04]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 00:08:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-23 00:34:25]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 00:35:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-23 00:47:09]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-11-23 00:47:11]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-23 03:00:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 03:00:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-11-23 03:19:17]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 03:19:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 04:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 04:46:10]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-12-19 23:20:46]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:21:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 23:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 23:44:50]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-12-19 23:50:58]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:51:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 00:17:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:17:54]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Sam McNally, 2016-12-20 00:45:17]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 00:45:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 01:03:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 01:03:07]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Sam McNally, 2016-12-20 01:26:40]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 01:27:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 01:53:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 01:53:50]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Sam McNally, 2016-12-20 02:11:58]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 02:12:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-20 04:57:28]

Patchset #7 (id:150001) has been deleted

[Sam McNally, 2016-12-20 04:57:32]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[Sam McNally, 2016-12-20 04:57:40]

Patchset #5 (id:110001) has been deleted

[Sam McNally, 2016-12-20 04:57:41]

Patchset #4 (id:90001) has been deleted

[Sam McNally, 2016-12-20 04:57:42]

Patchset #5 (id:130001) has been deleted

[Sam McNally, 2016-12-20 04:57:43]

Patchset #3 (id:70001) has been deleted

[Sam McNally, 2016-12-20 04:57:49]

Patchset #2 (id:50001) has been deleted

[Sam McNally, 2016-12-20 04:57:52]

Patchset #1 (id:30001) has been deleted

[commit-bot: I haz the power, 2016-12-20 04:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-20 04:58:16]

Patchset #2 (id:170001) has been deleted

[Sam McNally, 2016-12-20 04:58:19]

Patchset #1 (id:90001) has been deleted

[commit-bot: I haz the power, 2016-12-20 07:32:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 07:32:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sam McNally, 2016-12-20 10:02:50]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 10:03:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-20 10:56:45]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 10:57:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-20 11:00:11]

Patchset #1 (id:190001) has been deleted

[Sam McNally, 2016-12-20 11:08:13]

sammc@chromium.org changed reviewers:
+ bradnelson@chromium.org

[commit-bot: I haz the power, 2016-12-20 12:00:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 12:00:01]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-12-21 06:53:06]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 06:53:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 07:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 07:59:30]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2017-01-09 07:49:09]

Description was changed from

==========
Convert NaCl renderer-browser messages to mojo.

BUG=577685
==========

to

==========
Convert NaCl renderer-browser messages to mojo.

BUG=577685
==========

[bradnelson, 2017-01-09 07:49:09]

bradnelson@chromium.org changed reviewers:
+ bbudge@chromium.org, dschuff@chromium.org, mseaborn@chromium.org

[bradnelson, 2017-01-09 07:51:00]

Wow, that's quite a refactor.
I'm a little concerned about subtle issues around crashes, shutdown on windows,
but nothing jumped out.

Bill, Derek, Mark, did any of you want to take a look?

[Sam McNally, 2017-01-19 05:42:34]

ping

[Mark Seaborn, 2017-01-20 04:08:45]

I have two requests (sorry for the length):

1) Is there someone who is familiar with the Chrome IPC -> Mojo transition who
could do an initial review of changes like this, before handing the review over
to component owners?  That would be much easier.

I don't work actively on Chrome now and so I'm not very familiar with how Mojo
IPC works -- so I don't know the details of the threading model, object
ownership, etc.  Even when I've been more actively working on Chrome, Chrome IPC
is abstruse enough that some of those issues are hard to understand. :-)

I suspect a review by a Mojo/IPC reviewer might raise some questions about what
the code does which we component owners can then answer. It should lead to a
more thorough review.

2) Have you got any suggestions on how to approach reviewing a big CL like this?
 For example:

What types of conversion did you apply in this CL?  Some of it is obvious, like
changing the message handlers to Mojo format in nacl_host.h.  Other parts are
less obvious, like nacl_file_host.cc, where some functions now take a callback +
extra args instead of an IPC::Message.

Are there any docs on what issues arise for Chrome IPC -> Mojo conversions in
general?

What process did you go through to create this CL?  e.g. Was it incremental, or
did it only compile after you converted the last piece?  What do you want us
owners to look for?  Are there any parts you are less sure about that I should
look at more closely?

[Sam McNally, 2017-01-22 22:45:01]

sammc@chromium.org changed reviewers:
+ tibell@chromium.org

[Sam McNally, 2017-01-22 22:45:02]

+tibell for mojo conversion review

[Sam McNally, 2017-01-23 04:16:37]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 04:16:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 05:20:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 05:20:26]

Dry run: This issue passed the CQ dry run.

[tibell, 2017-01-31 01:24:13]

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.h (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/browse...
components/nacl/browser/nacl_file_host.h:44: const
scoped_refptr<base::TaskRunner>& origin_task_runner,
Could you document what |origin_task_runner| is for (e.g. you have to call
|callbacl on it)?

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
File components/nacl/renderer/nexe_load_manager.h (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
components/nacl/renderer/nexe_load_manager.h:36: NexeLoadManager(PP_Instance
instance, mojom::NaClHost* host);
Comment: |host| must outlive this object.

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
components/nacl/renderer/ppb_nacl_private_impl.cc:102:
base::LazyInstance<NaClHostConnectionHolder>::Leaky g_nacl_host_connection =
Comment why this global is needed?

[Sam McNally, 2017-02-06 23:01:11]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 23:02:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2017-02-07 00:13:19]

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.h (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/browse...
components/nacl/browser/nacl_file_host.h:44: const
scoped_refptr<base::TaskRunner>& origin_task_runner,
On 2017/01/31 01:24:12, tibell wrote:
> Could you document what |origin_task_runner| is for (e.g. you have to call
> |callbacl on it)?

Done.

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
File components/nacl/renderer/nexe_load_manager.h (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
components/nacl/renderer/nexe_load_manager.h:36: NexeLoadManager(PP_Instance
instance, mojom::NaClHost* host);
On 2017/01/31 01:24:13, tibell wrote:
> Comment: |host| must outlive this object.

Done.

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2514323004/diff/270001/components/nacl/render...
components/nacl/renderer/ppb_nacl_private_impl.cc:102:
base::LazyInstance<NaClHostConnectionHolder>::Leaky g_nacl_host_connection =
On 2017/01/31 01:24:13, tibell wrote:
> Comment why this global is needed?

Done.

[commit-bot: I haz the power, 2017-02-07 00:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:35:35]

Dry run: This issue passed the CQ dry run.

[tibell, 2017-02-08 04:38:13]

lgtm

[Sam McNally, 2017-02-22 06:36:04]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[Sam McNally, 2017-02-22 06:36:51]

Patchset #8 (id:350001) has been deleted

[commit-bot: I haz the power, 2017-02-22 06:37:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2017-02-22 06:52:49]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 06:53:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 09:06:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 09:06:06]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-02-22 23:52:10]

On 2017/01/20 04:08:45, Mark Seaborn wrote:
> I have two requests (sorry for the length):
> 
> 1) Is there someone who is familiar with the Chrome IPC -> Mojo transition who
> could do an initial review of changes like this, before handing the review
over
> to component owners?  That would be much easier.
> 
> I don't work actively on Chrome now and so I'm not very familiar with how Mojo
> IPC works -- so I don't know the details of the threading model, object
> ownership, etc.  Even when I've been more actively working on Chrome, Chrome
IPC
> is abstruse enough that some of those issues are hard to understand. :-)
> 
> I suspect a review by a Mojo/IPC reviewer might raise some questions about
what
> the code does which we component owners can then answer. It should lead to a
> more thorough review.
> 
> 2) Have you got any suggestions on how to approach reviewing a big CL like
this?
>  For example:
> 
> What types of conversion did you apply in this CL?  Some of it is obvious,
like
> changing the message handlers to Mojo format in nacl_host.h.  Other parts are
> less obvious, like nacl_file_host.cc, where some functions now take a callback
+
> extra args instead of an IPC::Message.

https://www.chromium.org/developers/design-documents/mojo/mojo-migration-guide
describes the general process.
https://www.chromium.org/developers/design-documents/mojo/type-mapping covers
type mapping.

The transformations involved on the existing code are:
- Sending message requires a connection rather than just an IPC::Sender =>
ppb_nacl_private_impl has a renderer-process-wide connection that the whole
process shares.
- Responses are modelled as callbacks instead of a separate response message =>
no MessageFilters needed in the renderer, just functions to handle the
responses.
- Sync and async responses are always callbacks rather than an IPC::Message*
that needs to be sent on the MessageFilter => implementation details are passed
a callback instead of a response message and MessageFilter.
- Responses have to be sent (by calling the callback) on the same thread that
received the request => implementation details are sometimes passed a TaskRunner
where the callback should be run.

> 
> Are there any docs on what issues arise for Chrome IPC -> Mojo conversions in
> general?

See the migration guide linked above, in particular
https://www.chromium.org/developers/design-documents/mojo/mojo-migration-guid....
In this case, the NaClHostMessageFilter replacement (NaClHostImpl) is still
ref-counted, but maintains a self-reference that's cleared when the mojo
connection breaks, providing similar lifetime to before.

Beyond that, ensuring that callbacks are called on the right thread is more
complicated than the thread-safe MessageFilter::Send of the old version.
> 
> What process did you go through to create this CL?  e.g. Was it incremental,
or
> did it only compile after you converted the last piece?  What do you want us
> owners to look for?  Are there any parts you are less sure about that I should
> look at more closely?

The steps were:
1. Generate a mojo interface from the IPC messages (merging response IPCs into
mojo message responses), using native-style typemapping to try to limit the CL
size.
2. Convert the browser side.
3. Convert the renderer side.
4. Delete old IPC messages.
4. Discover that native-style typemapping doesn't allow handles (files, other
channels, shared memory).
5. Add real mojo structs for structs containing handles.
6. Fixing up error cases (making bad message reporting work without a
MessageFilter, ensuring that every message expecting a response gets one)

The code compiled after each step, but didn't work until after step 5.

My main concerns would be with edge cases not covered by tests: handling errors
or invalid messages from the renderer.