Convert utility process out-of-process file patching IPC to mojo

Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add the interface to the utility process and expose it to
to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
Review-Url: https://codereview.chromium.org/2632143002
Cr-Commit-Position: refs/heads/master@{#445324}
Committed: https://chromium.googlesource.com/chromium/src/+/bfee08c642470b8a7d39988d718af6584f567adc

[Noel Gordon, 2017-01-16 07:08:56]

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

[commit-bot: I haz the power, 2017-01-16 07:09:10]

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

[Noel Gordon, 2017-01-16 08:15:49]

Description was changed from

==========
Convert utility process out-of-process file patching to mojo

BUG=597124, 660325
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

BUG=597124, 660325
==========

[commit-bot: I haz the power, 2017-01-16 09:09:22]

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

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

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-01-16 12:51:56]

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

[commit-bot: I haz the power, 2017-01-16 12:52:03]

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

[Noel Gordon, 2017-01-16 12:57:47]

Patchset #2 (id:20001) has been deleted

[Noel Gordon, 2017-01-16 12:58:52]

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

[commit-bot: I haz the power, 2017-01-16 12:59:03]

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

[Noel Gordon, 2017-01-16 13:13:34]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

BUG=597124, 660325
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 660325 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note the browser test now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124, 660325
==========

[Noel Gordon, 2017-01-16 13:14:22]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 660325 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note the browser test now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124, 660325
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 660325 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note: the browsertest now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124, 660325
==========

[commit-bot: I haz the power, 2017-01-16 13:14:44]

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

[commit-bot: I haz the power, 2017-01-16 13:14:44]

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

[Noel Gordon, 2017-01-16 13:24:47]

Patchset #2 (id:40001) has been deleted

[Noel Gordon, 2017-01-16 13:28:43]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 660325 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note: the browsertest now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124, 660325
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note: the browsertest now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

[Noel Gordon, 2017-01-16 13:30:24]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid issue 676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

Note: the browsertest now schedules all test work from the blocking
pool to match how out-of-process patching works in production.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid bug 676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

[Noel Gordon, 2017-01-16 13:33:08]

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

[commit-bot: I haz the power, 2017-01-16 13:33:11]

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-16 14:27:58]

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

[commit-bot: I haz the power, 2017-01-16 14:27:59]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-01-16 14:42:56]

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

[commit-bot: I haz the power, 2017-01-16 14:43:20]

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

[Noel Gordon, 2017-01-16 14:45:45]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move file and patch operations to the FILE thread
to avoid bug 676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

[Noel Gordon, 2017-01-16 14:53:45]

Patchset #3 (id:80001) has been deleted

[Noel Gordon, 2017-01-16 14:53:57]

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

[commit-bot: I haz the power, 2017-01-16 14:54: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, 2017-01-16 15:52:39]

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

[commit-bot: I haz the power, 2017-01-16 15:52:40]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-01-17 00:09:47]

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

[Noel Gordon, 2017-01-17 00:09:48]

https://chromium.googlesource.com/chromium/src.git/+/32afc44b332255130c247e3b...
reland attempt.

The patch was reverted in bug 676960 because it broke the component updater.  We
found out the hard way that mojo does not currently work from the blocking pool.

So to fix:

1) based on an idea from sammc@, change the browsertest to schedule the file
patch work from the blocking pool (which replicated the problem in bug 676960).

2) then move the mojo patch calls to a thread where it will work (I choose the
IO thread).  PTAL.

[Sam McNally, 2017-01-18 07:19:14]

lgtm

[Noel Gordon, 2017-01-18 12:50:02]

noel@chromium.org changed reviewers:
+ dcheng@chromium.org, waffles@chromium.org

[Noel Gordon, 2017-01-18 12:50:03]

+waffles for the out-of-process patching.
  and in particular, please check the browser test now matches how
out-to-process patching is used in production as you described to me on the
revert bug: sequenced task runner, blocking pool caller, etc.

+dcheng for IPC security.

[waffles, 2017-01-18 20:34:04]

Thanks for adding the browser test!

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
(right):

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:92:
task_runner_->PostTask(FROM_HERE, base::Bind(callback_, result));
I see you removed the

 if (task_runner_.get()) {
   ...
   task_runner_ = NULL
 }

wrapper.

We found that in practice sometimes the utility process could call us back
multiple times, later causing a crash when callback_ is run (because certain
objects could already be destructed). In particular, this could happen if
OnPatchFinished() but then OnProcessCrashed() (or vice versa).

Are we guaranteed that mojo (or an OOP attacker) cannot cause PatchDone to run
multiple times?

What is the behavior if the mojo process crashes? Will PatchDone ever be called?
(The purpose of running this patching out-of-process is to isolate ourselves
from crashes in the patcher, and it's not OK for such a crash to put the update
system into an unrecoverable state.)

[dcheng, 2017-01-18 23:05:00]

Is PS1 the original patch? I'm hoping to just diff the updates ;-)

[tibell, 2017-01-18 23:50:19]

lgtm

Nice test!

[Noel Gordon, 2017-01-19 13:25:29]

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc
(right):

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc:167:
RunPatchTest(update_client::kBsdiff, invalid, patch_file, output_file,
Since the patch file is courgette, it would be better to use
update_client::kCourgette here and in the other invalid file tests.  Doesn't
change the behavior of these tests though.

[Noel Gordon, 2017-01-19 13:29:18]

Patchset #4 (id:120001) has been deleted

[Noel Gordon, 2017-01-19 13:47:18]

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

[commit-bot: I haz the power, 2017-01-19 13:47:37]

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

[Noel Gordon, 2017-01-19 13:58:27]

On 2017/01/18 23:05:00, dcheng wrote:
> Is PS1 the original patch? I'm hoping to just diff the updates ;-)

Ah sorry no, and dumb of me not to think about doing that.  My apologies.

[commit-bot: I haz the power, 2017-01-19 13:58:58]

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

[commit-bot: I haz the power, 2017-01-19 13:58:59]

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

[Noel Gordon, 2017-01-19 13:59:46]

Patchset #4 (id:140001) has been deleted

[Noel Gordon, 2017-01-19 15:55:20]

Good questions:

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
(right):

https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:92:
task_runner_->PostTask(FROM_HERE, base::Bind(callback_, result));
On 2017/01/18 20:34:03, waffles (ooo Jan10 use sorin) wrote:
> I see you removed the
> 
>  if (task_runner_.get()) {
>    ...
>    task_runner_ = NULL
>  }
> 
> wrapper.
> 
> We found that in practice sometimes the utility process could call us back
> multiple times, later causing a crash when callback_ is run (because certain
> objects could already be destructed). In particular, this could happen if
> OnPatchFinished() but then OnProcessCrashed() (or vice versa).

Right, it could be racy.

> Are we guaranteed that mojo (or an OOP attacker) cannot cause PatchDone to run
> multiple times?

Yes.  And I talked over this issue in detail with Sam today.  When PatchDone is
called, note the utility_process_mojo_client_.reset() code line - this
termination instantly disconnects the mojo channel from the utility process (the
channel enters state disconnected), and also schedules the destruction of the
utility process.

If during this disconnect phase, a message arrived from the utility process, it
would be queued for delivery as normal.  Or if the utility process crashed, a
message would be queued for that.  However, delivery of these queued message(s)
(PatchDone callbacks in our case) gets blocked at mojo level: it
drops them the floor because the channel state is disconnected.  So we see a
PatchDone exactly once.

> What is the behavior if the mojo process crashes? Will PatchDone ever be
called?
> (The purpose of running this patching out-of-process is to isolate ourselves
> from crashes in the patcher, and it's not OK for such a crash to put the
update
> system into an unrecoverable state.)

Yes, PatchDone will be called with the error code you were using (-1).  This
handling must be setup before starting the utility process ...

70   utility_process_mojo_client_->set_error_callback(
71       base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this, -1));
72
73   utility_process_mojo_client_->Start();

by setting the error callback.  utility_process_mojo_client_ DCHECK's that an
error callback has been setup during Start().  If the utility process crashes at
any time, the remote end of our mojo channel to the utility process breaks,
which queues the error callback on our side of the mojo channel. Our side sees a
PatchDone(-1).

In summary, here are the queued messages that we could arrive on the mojo
channel

1) Normal case
   PatchDone(result)

2) Normal case and then the utility process crashed
   PatchDone(result), PatchDone(-1)

3) Utility process crashed case
   PatchDone(-1)

Again, we only service the first message queued and then disconnect the mojo
channel.  Hope I have explained well, but let me know.

[Noel Gordon, 2017-01-19 15:56:57]

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

[commit-bot: I haz the power, 2017-01-19 15:57:17]

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

[Noel Gordon, 2017-01-19 16:07:25]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

[commit-bot: I haz the power, 2017-01-19 16:55:47]

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

[commit-bot: I haz the power, 2017-01-19 16:55:49]

Dry run: This issue passed the CQ dry run.

[waffles, 2017-01-19 17:32:01]

On 2017/01/19 15:55:20, noel gordon wrote:
> Good questions:
> 
>
https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
> File
> chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
> (right):
> 
>
https://codereview.chromium.org/2632143002/diff/100001/chrome/browser/compone...
>
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:92:
> task_runner_->PostTask(FROM_HERE, base::Bind(callback_, result));
> On 2017/01/18 20:34:03, waffles (ooo Jan10 use sorin) wrote:
> > I see you removed the
> > 
> >  if (task_runner_.get()) {
> >    ...
> >    task_runner_ = NULL
> >  }
> > 
> > wrapper.
> > 
> > We found that in practice sometimes the utility process could call us back
> > multiple times, later causing a crash when callback_ is run (because certain
> > objects could already be destructed). In particular, this could happen if
> > OnPatchFinished() but then OnProcessCrashed() (or vice versa).
> 
> Right, it could be racy.
> 
> > Are we guaranteed that mojo (or an OOP attacker) cannot cause PatchDone to
run
> > multiple times?
> 
> Yes.  And I talked over this issue in detail with Sam today.  When PatchDone
is
> called, note the utility_process_mojo_client_.reset() code line - this
> termination instantly disconnects the mojo channel from the utility process
(the
> channel enters state disconnected), and also schedules the destruction of the
> utility process.
> 
> If during this disconnect phase, a message arrived from the utility process,
it
> would be queued for delivery as normal.  Or if the utility process crashed, a
> message would be queued for that.  However, delivery of these queued
message(s)
> (PatchDone callbacks in our case) gets blocked at mojo level: it
> drops them the floor because the channel state is disconnected.  So we see a
> PatchDone exactly once.
> 
> > What is the behavior if the mojo process crashes? Will PatchDone ever be
> called?
> > (The purpose of running this patching out-of-process is to isolate ourselves
> > from crashes in the patcher, and it's not OK for such a crash to put the
> update
> > system into an unrecoverable state.)
> 
> Yes, PatchDone will be called with the error code you were using (-1).  This
> handling must be setup before starting the utility process ...
> 
> 70   utility_process_mojo_client_->set_error_callback(
> 71       base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this, -1));
> 72
> 73   utility_process_mojo_client_->Start();
> 
> by setting the error callback.  utility_process_mojo_client_ DCHECK's that an
> error callback has been setup during Start().  If the utility process crashes
at
> any time, the remote end of our mojo channel to the utility process breaks,
> which queues the error callback on our side of the mojo channel. Our side sees
a
> PatchDone(-1).
> 
> In summary, here are the queued messages that we could arrive on the mojo
> channel
> 
> 1) Normal case
>    PatchDone(result)
> 
> 2) Normal case and then the utility process crashed
>    PatchDone(result), PatchDone(-1)
> 
> 3) Utility process crashed case
>    PatchDone(-1)
> 
> Again, we only service the first message queued and then disconnect the mojo
> channel.  Hope I have explained well, but let me know.

lgtm

Sounds great, thanks for the explanation and again for doing this work.

[dcheng, 2017-01-20 08:07:14]

mojo lgtm

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc
(right):

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc:31:
static base::FilePath test_file(const char* name) {
Nit: should be NamedLikeThis

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc:72:
return input_dir_.GetPath().AppendASCII("non-existant").AppendASCII(name);
Nit: non-existent ;-)

[Noel Gordon, 2017-01-20 09:40:08]

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

[commit-bot: I haz the power, 2017-01-20 09:40:17]

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

[Noel Gordon, 2017-01-20 09:46:25]

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc
(right):

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc:31:
static base::FilePath test_file(const char* name) {
On 2017/01/20 08:07:14, dcheng wrote:
> Nit: should be NamedLikeThis

Changed to TestFile(), done.

https://codereview.chromium.org/2632143002/diff/160001/chrome/browser/compone...
chrome/browser/component_updater/component_patcher_operation_out_of_process_browsertest.cc:72:
return input_dir_.GetPath().AppendASCII("non-existant").AppendASCII(name);
On 2017/01/20 08:07:14, dcheng wrote:
> Nit: non-existent ;-)

:) Fascinating etymology.  I do prefer the French root, but if existent is used,
then no hyphen in the antonym.

[Noel Gordon, 2017-01-20 09:52:09]

noel@chromium.org changed reviewers:
+ jochen@chromium.org

[Noel Gordon, 2017-01-20 09:52:09]

On 2017/01/20 09:46:25, noel gordon wrote:

> :) Fascinating etymology.  I do prefer the French root, but if existent is
used,
> then no hyphen in the antonym.

+jochen for OWNERS
  no hurry if you're travelling, will submit this nearer Monday AEST.

[commit-bot: I haz the power, 2017-01-20 10:54:28]

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

[commit-bot: I haz the power, 2017-01-20 10:54:30]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-20 13:07:52]

lgtm

[Noel Gordon, 2017-01-23 00:53:37]

On 2017/01/20 13:07:52, jochen wrote:
> lgtm

Right folks thanks.  Let's land and watch chrome canary for problems.

[Noel Gordon, 2017-01-23 00:53:45]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-01-23 00:53:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tibell@chromium.org, waffles@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2632143002/#ps180001
(title: "Final review comments.")

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

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 02:47:18]

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

[commit-bot: I haz the power, 2017-01-23 02:47:22]

Failed to apply patch for chrome/common/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/common/BUILD.gn:676
  error: chrome/common/BUILD.gn: patch does not apply

Patch:       chrome/common/BUILD.gn
Index: chrome/common/BUILD.gn
diff --git a/chrome/common/BUILD.gn b/chrome/common/BUILD.gn
index
ad33a7f72f1ccbf456a12aedb8731820ae322857..004516a75e4db2a5cb30a15141b169f3fe3e2766
100644
--- a/chrome/common/BUILD.gn
+++ b/chrome/common/BUILD.gn
@@ -676,6 +676,7 @@ mojom("mojo_bindings") {
   sources = [
     "conflicts/module_event_sink_win.mojom",
     "field_trial_recorder.mojom",
+    "file_patcher.mojom",
     "net_benchmarking.mojom",
     "network_diagnostics.mojom",
     "renderer_configuration.mojom",
@@ -685,6 +686,7 @@ mojom("mojo_bindings") {
 
   public_deps = [
     "//components/content_settings/core/common:mojo_bindings",
+    "//mojo/common:common_custom_types",
     "//url/mojo:url_mojom_gurl",
   ]
 }

[Noel Gordon, 2017-01-23 04:01:38]

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add that interface to the utility process, and expose the
interface to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add the interface to the utility process and expose it to
to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

[Noel Gordon, 2017-01-23 04:08:06]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-01-23 04:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, sammc@chromium.org, jochen@chromium.org,
tibell@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2632143002/#ps200001
(title: "Reupload to sync to tip.")

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

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:02:29]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485144486851100,
"parent_rev": "78610940a6934842e215e629956db0db80553c7e", "commit_rev":
"bfee08c642470b8a7d39988d718af6584f567adc"}

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

Description was changed from

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add the interface to the utility process and expose it to
to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124
==========

to

==========
Convert utility process out-of-process file patching IPC to mojo

Define interface chrome::mojom::FilePatcher for out-of-process file
patching. Add the interface to the utility process and expose it to
to the browser process by mojo policy [1].

ChromeOutOfProcessPatcher: remove PatchHost() helper class, replace
it with a content::UtilityProcessMojoClient() and use that to start
and stop the utility process needed for out-of-process file patches
and to perform out-of-process file patches using mojo calls, rather
than IPC::Send(). Move the mojo calls to the IO thread to avoid bug
676960 (mojo does not work from the blocking pool).

Add an in-process browsertest for the ChromeOutOfProcessPatcher (it
had no tests http://bit.ly/2h8mafX it seems). Copy the golden files
to random locations (input_file, patch_file) and patch them: checks
the patched output_file content matches the golden output file. Add
tests for invalid file inputs to the file patcher. Note all work in
the tests originates from the blocking pool: bug 676960.

[1] chrome_content_utility_manifest_overlay.json

BUG=597124

Review-Url: https://codereview.chromium.org/2632143002
Cr-Commit-Position: refs/heads/master@{#445324}
Committed:
https://chromium.googlesource.com/chromium/src/+/bfee08c642470b8a7d39988d718a...
==========

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

Committed patchset #6 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/bfee08c642470b8a7d39988d718a...