Convert utility process ParseMediaMetadata blob reading IPC to mojo

Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler() and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
Review-Url: https://codereview.chromium.org/2667443002
Cr-Commit-Position: refs/heads/master@{#450313}
Committed: https://chromium.googlesource.com/chromium/src/+/4ce9a407ae2f2299f9d62016fd5a4c71e3dc49d5

[Noel Gordon, 2017-01-29 11:43:09]

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

[commit-bot: I haz the power, 2017-01-29 11:43:22]

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-29 12:20:55]

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

[commit-bot: I haz the power, 2017-01-29 12:20:55]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-01-29 14:21:22]

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

[commit-bot: I haz the power, 2017-01-29 14:21:32]

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-29 14:31:38]

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

[commit-bot: I haz the power, 2017-01-29 14:31:39]

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_...)

[Noel Gordon, 2017-01-29 20:59:03]

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

[commit-bot: I haz the power, 2017-01-29 20:59:16]

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-29 20:59:52]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-29 21:41:02]

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

[commit-bot: I haz the power, 2017-01-29 21:41:03]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-03 12:56:33]

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

[commit-bot: I haz the power, 2017-02-03 12:56:50]

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-03 13:14:50]

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

[commit-bot: I haz the power, 2017-02-03 13:14:52]

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_...)
  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_ozone_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_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Noel Gordon, 2017-02-03 13:29:08]

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

[commit-bot: I haz the power, 2017-02-03 13:29: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, 2017-02-03 14:15:45]

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

[commit-bot: I haz the power, 2017-02-03 14:15:45]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-03 15:22:22]

Description was changed from

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

BUG=680928
==========

to

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into the existing IO/UI thread blob reading trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to send blob data IPC requests to the browser. This can go: ditch
  utility client AddHandler and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own that IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

[Noel Gordon, 2017-02-03 15:25:14]

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

[Noel Gordon, 2017-02-03 15:25:14]

+ sam and tibell for mojo
+ tommycli for media galleries
+ dcheng for IPC security.

[Noel Gordon, 2017-02-03 15:39:09]

Description was changed from

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into the existing IO/UI thread blob reading trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to send blob data IPC requests to the browser. This can go: ditch
  utility client AddHandler and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own that IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

to

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

[tommycli, 2017-02-03 17:17:29]

On 2017/02/03 15:25:14, noel gordon wrote:
> + sam and tibell for mojo
> + tommycli for media galleries
> + dcheng for IPC security.

From the media galleries perspective this looks fine, especially if all our
existing tests are passing. However, I know little about mojo -- so these
changes should be reviewed by someone else familiar with mojo (i.e. don't rely
on me to make sure it's an exact equivalent from the IPC version to mojo) -- but
from the media galleries perspective, this is lgtm.

[Sam McNally, 2017-02-03 22:14:16]

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:54: virtual
~SafeMediaMetadataParser();
Why is this protected and virtual instead of private and non-virtual?

https://codereview.chromium.org/2667443002/diff/80001/chrome/common/extension...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/common/extension...
chrome/common/extensions/media_parser.mojom:40: ReadBlob(int64 request_id, int64
position, int64 length)
Remove |request_id|.

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:15: :
media_data_source_(std::move(media_data_source)),
It's probably worth crashing if |media_data_source_| has an error.

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:60: struct
IPCDataSource::Request {
Bind |destination| and |callback| into the mojo callback instead of storing in a
map.

[Noel Gordon, 2017-02-05 02:31:48]

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

[commit-bot: I haz the power, 2017-02-05 02:32:16]

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-05 03:14:20]

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

[commit-bot: I haz the power, 2017-02-05 03:14:20]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-05 10:42:52]

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

[commit-bot: I haz the power, 2017-02-05 10:43:01]

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-05 11:38:42]

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

[commit-bot: I haz the power, 2017-02-05 11:38:43]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-05 23:45:04]

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:54: virtual
~SafeMediaMetadataParser();
On 2017/02/03 22:14:16, Sam McNally wrote:
> Why is this protected and virtual instead of private and non-virtual?

It could be done either way.  I picked they way we used on
https://codereview.chromium.org/2663603002

https://codereview.chromium.org/2667443002/diff/80001/chrome/common/extension...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/common/extension...
chrome/common/extensions/media_parser.mojom:40: ReadBlob(int64 request_id, int64
position, int64 length)
On 2017/02/03 22:14:16, Sam McNally wrote:
> Remove |request_id|.

Indeed, done.

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:15: :
media_data_source_(std::move(media_data_source)),
On 2017/02/03 22:14:16, Sam McNally wrote:
> It's probably worth crashing if |media_data_source_| has an error.

Ok, added a Crash() helper which calls IMMEDIATE_CRASH() from base/logging.h

https://codereview.chromium.org/2667443002/diff/80001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:60: struct
IPCDataSource::Request {
On 2017/02/03 22:14:16, Sam McNally wrote:
> Bind |destination| and |callback| into the mojo callback instead of storing in
a
> map.

Removed the map, and curried destination and the read callback into the mojo
callback.

[Sam McNally, 2017-02-06 00:03:39]

lgtm

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/2667443002/diff/80001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:54: virtual
~SafeMediaMetadataParser();
On 2017/02/05 23:45:03, noel gordon wrote:
> On 2017/02/03 22:14:16, Sam McNally wrote:
> > Why is this protected and virtual instead of private and non-virtual?
> 
> It could be done either way.  I picked they way we used on
> https://codereview.chromium.org/2663603002

In this case the destructor appears to be the only virtual method. Avoiding an
unnecessary vtable would be nice.

[Noel Gordon, 2017-02-06 01:37:19]

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

[commit-bot: I haz the power, 2017-02-06 01:37: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, 2017-02-06 02:18:40]

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

[commit-bot: I haz the power, 2017-02-06 02:18:41]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-06 03:13:56]

On 2017/02/06 00:03:39, Sam McNally wrote:
> lgtm

> In this case the destructor appears to be the only virtual method. Avoiding an
> unnecessary vtable would be nice.

Done.

[dcheng, 2017-02-06 06:36:48]

https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
chrome/common/extensions/media_parser.mojom:40: ReadBlob(int64 position, int64
length) => (array<uint8> data);
Is this something we could simplify to DataPipe in the future?

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
Why is this line needed?

[Noel Gordon, 2017-02-07 12:20:42]

https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
chrome/common/extensions/media_parser.mojom:40: ReadBlob(int64 position, int64
length) => (array<uint8> data);
On 2017/02/06 06:36:48, dcheng wrote:
> Is this something we could simplify to DataPipe in the future?

I've not used DataPipes thus far, but my anecdata about them is they are not
simple to use yet, they need some more work.

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
On 2017/02/06 06:36:48, dcheng wrote:
> Why is this line needed?

The browser process controls the utility process life-time now, and closes us
down in an orderly fashion.  No pipe connection error is seen in that case.

If the browser crashes, however, then we will see a connection error on the
media_data_source_ mojo pipe.  So we crash too since there's nothing more we can
do here if the browser process has gone.

[Noel Gordon, 2017-02-07 12:38:51]

On 2017/02/03 17:17:29, tommycli wrote:
> On 2017/02/03 15:25:14, noel gordon wrote:
> > + sam and tibell for mojo
> > + tommycli for media galleries
> > + dcheng for IPC security.
> 
> From the media galleries perspective this looks fine, especially if all our
> existing tests are passing. 

Unlike the other utility process IPC I've had to convert to mojo, media
galleries stuff comes with a test that exercises the utility process code, I
found it here: https://codereview.chromium.org/2614063003/#ps20001
MediaGalleriesPlatformAppBrowserTest.GetMetadata.

The test does a number of things, one of which seems to make the utility process
request blob data from a mp3 audio file [1] and then check the extracted
metadata that the utility process returns.  Thanks for writing the test.

[1] chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js

[tommycli, 2017-02-07 17:43:46]

On 2017/02/07 12:38:51, noel gordon wrote:
> On 2017/02/03 17:17:29, tommycli wrote:
> > On 2017/02/03 15:25:14, noel gordon wrote:
> > > + sam and tibell for mojo
> > > + tommycli for media galleries
> > > + dcheng for IPC security.
> > 
> > From the media galleries perspective this looks fine, especially if all our
> > existing tests are passing. 
> 
> Unlike the other utility process IPC I've had to convert to mojo, media
> galleries stuff comes with a test that exercises the utility process code, I
> found it here: https://codereview.chromium.org/2614063003/#ps20001
> MediaGalleriesPlatformAppBrowserTest.GetMetadata.
> 
> The test does a number of things, one of which seems to make the utility
process
> request blob data from a mp3 audio file [1] and then check the extracted
> metadata that the utility process returns.  Thanks for writing the test.
> 
> [1]
chrome/test/data/extensions/api_test/media_galleries/media_metadata/test.js

No problem. Good to see it paid off (finally?)

[dcheng, 2017-02-07 23:47:30]

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
On 2017/02/07 12:20:42, noel gordon wrote:
> On 2017/02/06 06:36:48, dcheng wrote:
> > Why is this line needed?
> 
> The browser process controls the utility process life-time now, and closes us
> down in an orderly fashion.  No pipe connection error is seen in that case.
> 
> If the browser crashes, however, then we will see a connection error on the
> media_data_source_ mojo pipe.  So we crash too since there's nothing more we
can
> do here if the browser process has gone.

Doesn't the browser crashing implicitly take down all the other processes? It
doesn't feel like a case we need to explicitly handle ourselves.

(I'm pretty sure the renderer makes no specific attempt to handle this case)

[Noel Gordon, 2017-02-07 23:53:00]

On 2017/02/07 23:47:30, dcheng wrote:
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> File chrome/utility/media_galleries/ipc_data_source.cc (right):
> 
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
> On 2017/02/07 12:20:42, noel gordon wrote:
> > On 2017/02/06 06:36:48, dcheng wrote:
> > > Why is this line needed?
> > 
> > The browser process controls the utility process life-time now, and closes
us
> > down in an orderly fashion.  No pipe connection error is seen in that case.
> > 
> > If the browser crashes, however, then we will see a connection error on the
> > media_data_source_ mojo pipe.  So we crash too since there's nothing more we
> can
> > do here if the browser process has gone.
> 
> Doesn't the browser crashing implicitly take down all the other processes? It
> doesn't feel like a case we need to explicitly handle ourselves.

Yeap, my understanding is that any decent OS would reap the Utility (child
process) if the browser (parent process) crashed.

@sammc did you have concerns that the above would not happen?

> (I'm pretty sure the renderer makes no specific attempt to handle this case)

[Sam McNally, 2017-02-08 00:45:59]

On 2017/02/07 23:53:00, noel gordon wrote:
> On 2017/02/07 23:47:30, dcheng wrote:
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > File chrome/utility/media_galleries/ipc_data_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
> > On 2017/02/07 12:20:42, noel gordon wrote:
> > > On 2017/02/06 06:36:48, dcheng wrote:
> > > > Why is this line needed?
> > > 
> > > The browser process controls the utility process life-time now, and closes
> us
> > > down in an orderly fashion.  No pipe connection error is seen in that
case.
> > > 
> > > If the browser crashes, however, then we will see a connection error on
the
> > > media_data_source_ mojo pipe.  So we crash too since there's nothing more
we
> > can
> > > do here if the browser process has gone.
> > 
> > Doesn't the browser crashing implicitly take down all the other processes?
It
> > doesn't feel like a case we need to explicitly handle ourselves.
> 
> Yeap, my understanding is that any decent OS would reap the Utility (child
> process) if the browser (parent process) crashed.
> 
> @sammc did you have concerns that the above would not happen?

I was mostly concerned about the browser dropping the connection for non-crash
reasons and this process silently hanging around forever.
> 
> > (I'm pretty sure the renderer makes no specific attempt to handle this case)

[dcheng, 2017-02-08 00:55:15]

On 2017/02/08 00:45:59, Sam McNally wrote:
> On 2017/02/07 23:53:00, noel gordon wrote:
> > On 2017/02/07 23:47:30, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > File chrome/utility/media_galleries/ipc_data_source.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
> > > On 2017/02/07 12:20:42, noel gordon wrote:
> > > > On 2017/02/06 06:36:48, dcheng wrote:
> > > > > Why is this line needed?
> > > > 
> > > > The browser process controls the utility process life-time now, and
closes
> > us
> > > > down in an orderly fashion.  No pipe connection error is seen in that
> case.
> > > > 
> > > > If the browser crashes, however, then we will see a connection error on
> the
> > > > media_data_source_ mojo pipe.  So we crash too since there's nothing
more
> we
> > > can
> > > > do here if the browser process has gone.
> > > 
> > > Doesn't the browser crashing implicitly take down all the other processes?
> It
> > > doesn't feel like a case we need to explicitly handle ourselves.
> > 
> > Yeap, my understanding is that any decent OS would reap the Utility (child
> > process) if the browser (parent process) crashed.
> > 
> > @sammc did you have concerns that the above would not happen?
> 
> I was mostly concerned about the browser dropping the connection for non-crash
> reasons and this process silently hanging around forever.
> > 
> > > (I'm pretty sure the renderer makes no specific attempt to handle this
case)

I would consider that a process management bug in the browser though.

[Noel Gordon, 2017-02-08 03:29:13]

On 2017/02/08 00:55:15, dcheng wrote:
> On 2017/02/08 00:45:59, Sam McNally wrote:
> > On 2017/02/07 23:53:00, noel gordon wrote:
> > > On 2017/02/07 23:47:30, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > > File chrome/utility/media_galleries/ipc_data_source.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > > chrome/utility/media_galleries/ipc_data_source.cc:63: IMMEDIATE_CRASH();
> > > > On 2017/02/07 12:20:42, noel gordon wrote:
> > > > > On 2017/02/06 06:36:48, dcheng wrote:
> > > > > > Why is this line needed?
> > > > > 
> > > > > The browser process controls the utility process life-time now, and
> closes
> > > us
> > > > > down in an orderly fashion.  No pipe connection error is seen in that
> > case.
> > > > > 
> > > > > If the browser crashes, however, then we will see a connection error
on
> > the
> > > > > media_data_source_ mojo pipe.  So we crash too since there's nothing
> more
> > we
> > > > can
> > > > > do here if the browser process has gone.
> > > > 
> > > > Doesn't the browser crashing implicitly take down all the other
processes?
> > It
> > > > doesn't feel like a case we need to explicitly handle ourselves.
> > > 
> > > Yeap, my understanding is that any decent OS would reap the Utility (child
> > > process) if the browser (parent process) crashed.
> > > 
> > > @sammc did you have concerns that the above would not happen?
> > 
> > I was mostly concerned about the browser dropping the connection for
non-crash
> > reasons and this process silently hanging around forever.

IC, such as some silly error in browser process.

> > > > (I'm pretty sure the renderer makes no specific attempt to handle this
> case)
> 
> I would consider that a process management bug in the browser though.

So do we crash or not?  I would expect this forced crash would create a log,
right?  If so, that'd help finger the assumed process management bug.

[dcheng, 2017-02-08 03:34:13]

On 2017/02/08 03:29:13, noel gordon wrote:
> On 2017/02/08 00:55:15, dcheng wrote:
> > On 2017/02/08 00:45:59, Sam McNally wrote:
> > > On 2017/02/07 23:53:00, noel gordon wrote:
> > > > On 2017/02/07 23:47:30, dcheng wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > > > File chrome/utility/media_galleries/ipc_data_source.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/utility/media_g...
> > > > > chrome/utility/media_galleries/ipc_data_source.cc:63:
IMMEDIATE_CRASH();
> > > > > On 2017/02/07 12:20:42, noel gordon wrote:
> > > > > > On 2017/02/06 06:36:48, dcheng wrote:
> > > > > > > Why is this line needed?
> > > > > > 
> > > > > > The browser process controls the utility process life-time now, and
> > closes
> > > > us
> > > > > > down in an orderly fashion.  No pipe connection error is seen in
that
> > > case.
> > > > > > 
> > > > > > If the browser crashes, however, then we will see a connection error
> on
> > > the
> > > > > > media_data_source_ mojo pipe.  So we crash too since there's nothing
> > more
> > > we
> > > > > can
> > > > > > do here if the browser process has gone.
> > > > > 
> > > > > Doesn't the browser crashing implicitly take down all the other
> processes?
> > > It
> > > > > doesn't feel like a case we need to explicitly handle ourselves.
> > > > 
> > > > Yeap, my understanding is that any decent OS would reap the Utility
(child
> > > > process) if the browser (parent process) crashed.
> > > > 
> > > > @sammc did you have concerns that the above would not happen?
> > > 
> > > I was mostly concerned about the browser dropping the connection for
> non-crash
> > > reasons and this process silently hanging around forever.
> 
> IC, such as some silly error in browser process.
> 
> > > > > (I'm pretty sure the renderer makes no specific attempt to handle this
> > case)
> > 
> > I would consider that a process management bug in the browser though.
> 
> So do we crash or not?  I would expect this forced crash would create a log,
> right?  If so, that'd help finger the assumed process management bug.

I would prefer to see this removed.

[tibell, 2017-02-08 04:33:00]

lgtm

[Noel Gordon, 2017-02-08 05:01:31]

On 2017/02/08 03:34:13, dcheng wrote:
> On 2017/02/08 03:29:13, noel gordon wrote:

> > So do we crash or not?  I would expect this forced crash would create a log,
> > right?  If so, that'd help finger the assumed process management bug.
> 
> I would prefer to see this removed.

Ok, done.

[Noel Gordon, 2017-02-08 05:01:43]

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

[commit-bot: I haz the power, 2017-02-08 05:01:55]

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-08 05:33:51]

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

[commit-bot: I haz the power, 2017-02-08 05:33:52]

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

[dcheng, 2017-02-08 08:33:39]

mojo lgtm

[Noel Gordon, 2017-02-08 10:04:00]

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

[commit-bot: I haz the power, 2017-02-08 10:04: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, 2017-02-08 10:30:28]

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

[commit-bot: I haz the power, 2017-02-08 10:30:29]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-08 13:12:12]

On 2017/02/06 06:36:48, dcheng wrote:
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
> File chrome/common/extensions/media_parser.mojom (right):
> 
>
https://codereview.chromium.org/2667443002/diff/140001/chrome/common/extensio...
> chrome/common/extensions/media_parser.mojom:40: ReadBlob(int64 position, int64
> length) => (array<uint8> data);
> Is this something we could simplify to DataPipe in the future?

More to this, looked into it.  DataPipe is designed for streaming data, whereas
the metadata blob reader can read a part of a blob data, and then seek backwards
to read an earlier section in the blob.  Not something DataPipe can do, so I
think the answer is no.

There is a File interface in mojo (not mojo's base::File, mind) that allows
seeking anywhere in the File data.  But it's all full of TODO and currently
unfinished [1].

[1]
https://cs.chromium.org/chromium/src/components/filesystem/public/interfaces/...

[Noel Gordon, 2017-02-09 04:05:44]

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

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

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-09 04:35:43]

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

[commit-bot: I haz the power, 2017-02-09 04:35:45]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-10 05:35:25]

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

[commit-bot: I haz the power, 2017-02-10 05:35:53]

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-10 06:51:07]

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

[commit-bot: I haz the power, 2017-02-10 06:51:08]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-13 00:28:35]

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

[Noel Gordon, 2017-02-13 00:28:36]

Synced up to tip, +jochen for OWNERS

[Noel Gordon, 2017-02-13 03:46:10]

Description was changed from

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

to

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler() and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

[jochen (gone - plz use gerrit), 2017-02-14 09:03:36]

lgtm

[Noel Gordon, 2017-02-14 09:06:12]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-02-14 09:06:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, sammc@chromium.org, tibell@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2667443002/#ps180001
(title: "Sync to ToT and merge in dependent patch changes.")

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

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-14 10:29:06]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487063172695920,
"parent_rev": "f07ca90e103c3d76fd7e2c8d44621e8040c184cf", "commit_rev":
"4ce9a407ae2f2299f9d62016fd5a4c71e3dc49d5"}

[commit-bot: I haz the power, 2017-02-14 10:29:40]

Description was changed from

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler() and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928
==========

to

==========
Convert utility process ParseMediaMetadata blob reading IPC to mojo

Update mojo ParseMediaMetadata API
  Add API documentation. Add a new MediaDataSource interface to the API and
  document: allow the utility process to request blob data for parsing from
  the browser process using mojo, rather than IPC.

Use content::UtilityProcessMojoClient
  The code used content::UtilityProcessHost to control the utility process.
  Replace that with the mojo utility client, remove parser state, implement
  the MediaDataSource (used to handle utility process blob data requests by
  calling into existing IO/UI thread blob reader trampolines).

Remove the utility process AddHandler
  ParseMediaMetadata needed the utility client so it could hook its message
  handler to receive IPC blob data responses from the browser. This can now
  go: ditch utility client AddHandler() and related.

Update metadata::IPCDataSource, MediaMetadataParser
  Make IPCDataSource own the MediaDataSource and use it to request the blob
  data. In turn, make MediaMetadataParser own the IPCDataSource.

TEST=Covered by MediaGalleriesPlatformAppBrowserTest.GetMetadata
BUG=680928

Review-Url: https://codereview.chromium.org/2667443002
Cr-Commit-Position: refs/heads/master@{#450313}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ce9a407ae2f2299f9d62016fd5a...
==========

[commit-bot: I haz the power, 2017-02-14 10:29:42]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/4ce9a407ae2f2299f9d62016fd5a...