Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-07 15:51:04]

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

[commit-bot: I haz the power, 2017-03-07 15:52: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-03-07 15:57:59]

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

[commit-bot: I haz the power, 2017-03-07 15:58:00]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  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-...)
  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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Noel Gordon, 2017-03-07 16:14:06]

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

[commit-bot: I haz the power, 2017-03-07 16:14:48]

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-03-07 16:16:11]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-07 16:19:51]

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

[commit-bot: I haz the power, 2017-03-07 16:19:52]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Noel Gordon, 2017-03-07 16:45:25]

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

[commit-bot: I haz the power, 2017-03-07 16:46: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, 2017-03-07 16:53:29]

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

[commit-bot: I haz the power, 2017-03-07 16:53:29]

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

[Noel Gordon, 2017-03-07 16:58:35]

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

[commit-bot: I haz the power, 2017-03-07 16:59:33]

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-03-07 17:06:18]

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

[commit-bot: I haz the power, 2017-03-07 17:06:19]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Noel Gordon, 2017-03-08 02:06:11]

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

[commit-bot: I haz the power, 2017-03-08 02:07:28]

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-03-08 03:58:13]

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

[commit-bot: I haz the power, 2017-03-08 03:58:14]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-03-08 04:51:45]

Patchset #2 (id:40001) has been deleted

[Noel Gordon, 2017-03-08 04:52:43]

Patchset #2 (id:60001) has been deleted

[Noel Gordon, 2017-03-08 06:50:06]

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

[commit-bot: I haz the power, 2017-03-08 06:50:23]

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-03-08 07:10:52]

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

[commit-bot: I haz the power, 2017-03-08 07:10:53]

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

[Noel Gordon, 2017-03-08 07:33:42]

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

[commit-bot: I haz the power, 2017-03-08 07:34: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-03-08 07:45:19]

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

[commit-bot: I haz the power, 2017-03-08 07:45:20]

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

[Noel Gordon, 2017-03-08 08:00:33]

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

[commit-bot: I haz the power, 2017-03-08 08:01:09]

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-03-08 08:57:36]

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

[commit-bot: I haz the power, 2017-03-08 08:57:59]

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-03-08 09:42:53]

Description was changed from

==========
Convert utility process Safe Browsing ZIP / DMG Analyzer IPC to mojo

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in clients and unit tests (fix the DMG unit test).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-08 09:55:19]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in clients and unit tests (fix the DMG unit test).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in clients and the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-08 10:11:44]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in clients and the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in both clients and in the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-08 10:12:18]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in both clients and in the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in both clients and in the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo will be called by these tests.

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

[commit-bot: I haz the power, 2017-03-08 10:15:09]

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

[commit-bot: I haz the power, 2017-03-08 10:15:10]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-03-08 10:20:29]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temporary file clean-up code (not neeed since
the utility process now owns the files). ResultCallback: use it
in both clients and in the DMG unit test.

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo will be called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-08 10:22:09]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze result.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[Noel Gordon, 2017-03-08 10:51:53]

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

[commit-bot: I haz the power, 2017-03-08 10:52: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-03-08 11:03:26]

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

[Noel Gordon, 2017-03-08 11:03:27]

sammc, tibell for the mojo and client class updates
 - client class impls were not quite the same, I made
   them more similar, future work might be for the
   owners of these clients to have one client maybe
 - file analyze results contain a protobuf, go [Native]
   with traits to handle that. 
 - thanks for the help with the build files to get
   the native trait to work.
dcheng
  - IPC security
jochen
  - OWNERS

[Noel Gordon, 2017-03-08 11:05:00]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze result.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

[commit-bot: I haz the power, 2017-03-08 11:49:14]

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

[commit-bot: I haz the power, 2017-03-08 11:49:15]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-08 18:52:02]

i'll wait for sammc & tibell to finish their reviews

[tibell, 2017-03-08 23:55:47]

lgtm

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/safe_arc...
File chrome/common/safe_archive_analyzer.mojom (right):

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/safe_arc...
chrome/common/safe_archive_analyzer.mojom:15: // download protection, given a
|temporary_file| used to extract files
The |temporary_file| confuses me. What is it for? I'd expect a temporary
directory to unpack the zip file into, not a file.

[Noel Gordon, 2017-03-09 02:24:59]

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/safe_arc...
File chrome/common/safe_archive_analyzer.mojom (right):

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/safe_arc...
chrome/common/safe_archive_analyzer.mojom:15: // download protection, given a
|temporary_file| used to extract files
On 2017/03/08 23:55:47, tibell wrote:
> The |temporary_file| confuses me. What is it for? I'd expect a temporary
> directory to unpack the zip file into, not a file.

Given the chrome_utility_messages.h API, they only required a temporary _file_,
not a directory, so I assume (/me not looking at their code) that they extract
files from the extension zip one after the other: extract file, check file, move
on to the next file.

Shame that a temporary file needs to be on this API, but since this code runs in
a sandboxed utility process (not allowed to open files), the API has to provide
the temporary file they need to get the job done.

[Sam McNally, 2017-03-09 08:38:58]

lgtm

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:522: # the sources += [...] protobuf include files.
Is the "sources += [...]" part intended?

[Noel Gordon, 2017-03-09 08:47:34]

On 2017/03/09 08:38:58, Sam McNally wrote:
> lgtm
> 
> https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn
> File chrome/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn...
> chrome/common/BUILD.gn:522: # the sources += [...] protobuf include files.
> Is the "sources += [...]" part intended?

Yeah, I was trying to refer to the sources += [...] just above, where the
includes files we depend on are listed.  Got a better suggestion?

[Noel Gordon, 2017-03-09 08:53:21]

https://codereview.chromium.org/2737763002/diff/180001/chrome/typemaps.gni
File chrome/typemaps.gni (right):

https://codereview.chromium.org/2737763002/diff/180001/chrome/typemaps.gni#ne...
chrome/typemaps.gni:6: "//chrome/common/safe_archive_analyzer.typemap",
Also, this part vexed me.  Just shoved it in here since this .gni file existed. 
I noticed instant.mojo near my changes to  in chrome/common/BUILD.gn, and this
is where they put their typemap.

Alternative would to be to add chrome/common/typemaps.gni and add put my typemap
therein.

[Sam McNally, 2017-03-09 08:54:39]

On 2017/03/09 08:47:34, noel gordon wrote:
> On 2017/03/09 08:38:58, Sam McNally wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn
> > File chrome/common/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn...
> > chrome/common/BUILD.gn:522: # the sources += [...] protobuf include files.
> > Is the "sources += [...]" part intended?
> 
> Yeah, I was trying to refer to the sources += [...] just above, where the
> includes files we depend on are listed.  Got a better suggestion?

How about something like the following?
# safe_archive_analyzer.mojom has a [Native] trait that depends on
# the protobuf headers in the sources list above.

[Noel Gordon, 2017-03-09 08:56:19]

On 2017/03/09 08:54:39, Sam McNally wrote:
> On 2017/03/09 08:47:34, noel gordon wrote:
> > On 2017/03/09 08:38:58, Sam McNally wrote:
> > > lgtm
> > > 
> > >
> https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn
> > > File chrome/common/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn...
> > > chrome/common/BUILD.gn:522: # the sources += [...] protobuf include files.
> > > Is the "sources += [...]" part intended?
> > 
> > Yeah, I was trying to refer to the sources += [...] just above, where the
> > includes files we depend on are listed.  Got a better suggestion?
> 
> How about something like the following?
> # safe_archive_analyzer.mojom has a [Native] trait that depends on
> # the protobuf headers in the sources list above.

Sounds good to me.  I will make that change.

[Sam McNally, 2017-03-09 08:58:28]

https://codereview.chromium.org/2737763002/diff/180001/chrome/typemaps.gni
File chrome/typemaps.gni (right):

https://codereview.chromium.org/2737763002/diff/180001/chrome/typemaps.gni#ne...
chrome/typemaps.gni:6: "//chrome/common/safe_archive_analyzer.typemap",
On 2017/03/09 08:53:21, noel gordon wrote:
> Also, this part vexed me.  Just shoved it in here since this .gni file
existed. 
> I noticed instant.mojo near my changes to  in chrome/common/BUILD.gn, and this
> is where they put their typemap.
> 
> Alternative would to be to add chrome/common/typemaps.gni and add put my
typemap
> therein.

There aren't any guidelines that I know of on how fine-grained typemaps.gni
files should be. Perhaps we could improve that.

[Noel Gordon, 2017-03-09 09:13:11]

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

[commit-bot: I haz the power, 2017-03-09 09:13:33]

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-03-09 09:14:25]

On 2017/03/09 08:56:19, noel gordon wrote:
> On 2017/03/09 08:54:39, Sam McNally wrote:
> > On 2017/03/09 08:47:34, noel gordon wrote:
> > > On 2017/03/09 08:38:58, Sam McNally wrote:
>
https://codereview.chromium.org/2737763002/diff/180001/chrome/common/BUILD.gn...
> > > > chrome/common/BUILD.gn:522: # the sources += [...] protobuf include
files.
> > > > Is the "sources += [...]" part intended?
> > > 
> > > Yeah, I was trying to refer to the sources += [...] just above, where the
> > > includes files we depend on are listed.  Got a better suggestion?
> > 
> > How about something like the following?
> > # safe_archive_analyzer.mojom has a [Native] trait that depends on
> > # the protobuf headers in the sources list above.
> 
> Sounds good to me.  I will make that change.

Done.

[commit-bot: I haz the power, 2017-03-09 10:11:52]

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

[commit-bot: I haz the power, 2017-03-09 10:11:53]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-09 11:29:51]

lgtm

[dcheng, 2017-03-10 04:13:43]

lgtm with nits

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.h (right):

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.h:18: using Results =
zip_analyzer::Results;
Nit: no using declaration in header (Results is a fairly generic name, and while
it's namespaced under safe_browsing, it's probably best to just explicitly
qualify it: we had some recent breakage due to using declarations in Blink)
Moving this inside the class decl would be one way to resolve potential
ambiguity.

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.h (right):

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.h:18: using Results =
zip_analyzer::Results;
Ditto.

[Noel Gordon, 2017-03-10 14:03:15]

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

[commit-bot: I haz the power, 2017-03-10 14:03:28]

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-03-10 14:07:01]

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

[commit-bot: I haz the power, 2017-03-10 14:07:02]

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

[Noel Gordon, 2017-03-10 14:42:43]

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

[commit-bot: I haz the power, 2017-03-10 14:42:54]

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-03-10 14:53:17]

Bots went red here.  Analyze step failed most bots.  Will look a that next.

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.h (right):

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.h:18: using Results =
zip_analyzer::Results;
On 2017/03/10 04:13:43, dcheng wrote:
> Nit: no using declaration in header (Results is a fairly generic name, and
while
> it's namespaced under safe_browsing, it's probably best to just explicitly
> qualify it: we had some recent breakage due to using declarations in Blink)

Right.

> Moving this inside the class decl would be one way to resolve potential
> ambiguity.

Done.

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/sandboxed_zip_analyzer.h (right):

https://codereview.chromium.org/2737763002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/sandboxed_zip_analyzer.h:18: using Results =
zip_analyzer::Results;
On 2017/03/10 04:13:43, dcheng wrote:
> Ditto.

Ditto Done.

[Noel Gordon, 2017-03-10 15:00:33]

On 2017/03/10 14:53:17, noel gordon wrote:
> Bots went red here.  Analyze step failed most bots.  Will look at that next.

bot % gen //out/Debug --check
ERROR at //chrome/common/safe_browsing/zip_analyzer.h:12:11: Include not
allowed.
#include "components/safe_browsing/csd.pb.h"
          ^--------------------------------
It is not in any dependency of
  //chrome/common:mojo_bindings
The include file is in the target(s):
  //components/safe_browsing:csd_proto
  //components/safe_browsing:csd_proto_gen
at least one of which should somehow be reachable.
GN gen failed: 1

Added the first, //components/safe_browsing:csd_proto, to
safe_archive_analyzer.typemap 

deps = [
  "//chrome/common/safe_browsing:proto",
+ "//components/safe_browsing:csd_proto",
]

and % gen check out/Release no longer complains.  Bots trying that now patch set
#10.

[Noel Gordon, 2017-03-10 15:19:31]

https://codereview.chromium.org/2737763002/diff/240001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2737763002/diff/240001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:153:
DCHECK(temporary_file.IsValid());
@sammc, these DCHECK are redundant, no?  Mojo won't tx/rx an invalid base::File,
IIRC (blocked at type validation time).

[Noel Gordon, 2017-03-10 15:38:51]

https://codereview.chromium.org/2737763002/diff/240001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2737763002/diff/240001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:153:
DCHECK(temporary_file.IsValid());
On 2017/03/10 15:19:31, noel gordon wrote:
> @sammc, these DCHECK are redundant, no?  Mojo won't tx/rx an invalid
base::File,
> IIRC (blocked at type validation time).

Reading traits, mojo can rx a kInvalidPlatformFile here.  The two callers we
have bail if given invalid files, they never send them here.

[commit-bot: I haz the power, 2017-03-10 15:41:24]

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

[commit-bot: I haz the power, 2017-03-10 15:41:25]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-03-11 01:57:46]

Bot all green, will try to land.

[Noel Gordon, 2017-03-11 01:57:56]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-03-11 01:57:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tibell@chromium.org, jochen@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2737763002/#ps240001
(title: "Add //components/safe_browsing:csd_proto to typemap deps.")

[commit-bot: I haz the power, 2017-03-11 01:58:09]

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-03-11 02:03:34]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489197476318640,
"parent_rev": "eaeb5c6fe84d2ae813eef86e44a85b767c53b612", "commit_rev":
"31e7b9090b8a1301e5c51a47f5bbd5bbe37e8914"}

[commit-bot: I haz the power, 2017-03-11 02:04:17]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

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

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

BUG=597124

Review-Url: https://codereview.chromium.org/2737763002
Cr-Commit-Position: refs/heads/master@{#456255}
Committed:
https://chromium.googlesource.com/chromium/src/+/31e7b9090b8a1301e5c51a47f5bb...
==========

[commit-bot: I haz the power, 2017-03-11 02:04:19]

Committed patchset #10 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/31e7b9090b8a1301e5c51a47f5bb...

[Noel Gordon, 2017-03-11 06:15:01]

On 2017/03/11 02:04:19, commit-bot: I haz the power wrote:
> Committed patchset #10 (id:240001) as
>
https://chromium.googlesource.com/chromium/src/+/31e7b9090b8a1301e5c51a47f5bb...

Pinged on https://crbug.com/696529#c14 ... there are intermittent failures on
the Mac builder due to a failed compile step

[1912/49531] ACTION //chrome/test:mac_safe_browsing_test_data

https://crbug.com/696529 seems to cover the issues, +cc rsesek@ on this CL just
in case the change made the problem worse.

[Noel Gordon, 2017-03-11 06:15:21]

Description was changed from

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

BUG=597124

Review-Url: https://codereview.chromium.org/2737763002
Cr-Commit-Position: refs/heads/master@{#456255}
Committed:
https://chromium.googlesource.com/chromium/src/+/31e7b9090b8a1301e5c51a47f5bb...
==========

to

==========
Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

BUG=597124

Review-Url: https://codereview.chromium.org/2737763002
Cr-Commit-Position: refs/heads/master@{#456255}
Committed:
https://chromium.googlesource.com/chromium/src/+/31e7b9090b8a1301e5c51a47f5bb...
==========

[Noel Gordon, 2017-03-11 06:15:21]

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

[Noel Gordon, 2017-03-15 11:11:14]

noel@chromium.org changed reviewers:
- rsesek@chromium.org