Shape Detection: move the Mac service to the sandboxed GPU process

Shape Detection: move the Mac service to the sandboxed GPU process

ShapeDetection in Mac uses privileged and potentially dangerous APIs
inside CoreImage and perhaps more, transitively.  This CL moves the 
shape detection service to run in the GPU process, which is sandboxed.

The reason for using the GPU process is that CoreImage APIs seem
to depend strongly and mysteriously on the GPU, as I learned while
trying to get the unittests to work [1] in our normal mac bots (which 
are virtual machines and have no GPU).  I tried using a sw rendering
context and a coupe of other circumventions but nothing really worked.

[1] early PSs in https://codereview.chromium.org/2677553003/

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2739163003
Cr-Commit-Position: refs/heads/master@{#456555}
Committed: https://chromium.googlesource.com/chromium/src/+/95d98937e0fb0baf11d7c3088a99bb20a58e32cf

[mcasas, 2017-03-09 22:18:56]

Description was changed from

==========
Shape Detection: move the Mac service to sandbox GPU process [wip]


BUG=666143
==========

to

==========
Shape Detection: move the Mac service to sandbox GPU process [wip]

wip

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[mcasas, 2017-03-09 23:27:14]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-03-09 23:50:45]

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

[commit-bot: I haz the power, 2017-03-09 23:51: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-10 01:21:59]

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

[commit-bot: I haz the power, 2017-03-10 01:22:00]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-03-10 02:07:18]

Description was changed from

==========
Shape Detection: move the Mac service to sandbox GPU process [wip]

wip

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Shape Detection: move the Mac service to the sandboxed GPU process

ShapeDetection in Mac uses privileged and potentially dangerous APIs
inside CoreImage and perhaps more, transitively.  This CL moves the 
shape detection service to run in the GPU process, which is sandboxed.

The reason for using the GPU process is that CoreImage APIs seem
to depend strongly and mysteriously on the GPU, as I learned while
trying to get the unittests to work [1] in our normal mac bots (which 
are virtual machines and have no GPU).  I tried using a sw rendering
context and a coupe of other circumventions but nothing really worked.

[1] early PSs in https://codereview.chromium.org/2677553003/

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[mcasas, 2017-03-10 02:08:52]

mcasas@chromium.org changed reviewers:
+ kbr@chromium.org, rockot@chromium.org

[mcasas, 2017-03-10 02:08:53]

rockot@ what we worked on today, PTAL 
(also please advise on the BUILD.gn dependency)

kbr@ RS plz /PTAL

rsesek@: FYI/ptal

https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn
File content/gpu/BUILD.gn (right):

https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn#ne...
content/gpu/BUILD.gn:65: "//services/shape_detection:lib",
This is because I need 
 shape_detection::ShapeDetectionService::Create() 
which is defined in 
 services/shape_detection/shape_detection_service.h
perhaps I should move this last file to
 services/shape_detection/public/cpp ?

[Ken Rockot(use gerrit already), 2017-03-10 16:09:12]

nice, LGTM

https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn
File content/gpu/BUILD.gn (right):

https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn#ne...
content/gpu/BUILD.gn:65: "//services/shape_detection:lib",
On 2017/03/10 at 02:08:53, mcasas wrote:
> This is because I need 
>  shape_detection::ShapeDetectionService::Create() 
> which is defined in 
>  services/shape_detection/shape_detection_service.h
> perhaps I should move this last file to
>  services/shape_detection/public/cpp ?

Nooooo. The lib is the internal implementation details of the service. It should
never be part of "public". You have to link against it in content because you're
embedding the service in content, so this is exactly what you want.

[Ken Rockot(use gerrit already), 2017-03-10 16:12:03]

On 2017/03/10 at 16:09:12, Ken Rockot wrote:
> nice, LGTM
> 
> https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn
> File content/gpu/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2739163003/diff/20001/content/gpu/BUILD.gn#ne...
> content/gpu/BUILD.gn:65: "//services/shape_detection:lib",
> On 2017/03/10 at 02:08:53, mcasas wrote:
> > This is because I need 
> >  shape_detection::ShapeDetectionService::Create() 
> > which is defined in 
> >  services/shape_detection/shape_detection_service.h
> > perhaps I should move this last file to
> >  services/shape_detection/public/cpp ?
> 
> Nooooo. The lib is the internal implementation details of the service. It
should never be part of "public". You have to link against it in content because
you're embedding the service in content, so this is exactly what you want.

In fact we should probably have a default blanket rule that
//services/foo/public/DEPS explicitly forbid anything from //services/foo apart
from //services/foo/public. Exposing a transitive dependency on the service
implementation from the public library completely violates the principal intent
of making the thing a service.

[Robert Sesek, 2017-03-10 21:35:39]

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

[Robert Sesek, 2017-03-10 21:35:40]

GPU process seems like a reasonable place for this. LGTM

[Ken Russell (switch to Gerrit), 2017-03-13 22:28:37]

rs lgtm

[mcasas, 2017-03-13 22:37:18]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2017-03-13 22:38:30]

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-13 23:55:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489444638873550,
"parent_rev": "6823b024d53b45b8231a1d4a8e16a463327b2e63", "commit_rev":
"95d98937e0fb0baf11d7c3088a99bb20a58e32cf"}

[commit-bot: I haz the power, 2017-03-13 23:56:24]

Description was changed from

==========
Shape Detection: move the Mac service to the sandboxed GPU process

ShapeDetection in Mac uses privileged and potentially dangerous APIs
inside CoreImage and perhaps more, transitively.  This CL moves the 
shape detection service to run in the GPU process, which is sandboxed.

The reason for using the GPU process is that CoreImage APIs seem
to depend strongly and mysteriously on the GPU, as I learned while
trying to get the unittests to work [1] in our normal mac bots (which 
are virtual machines and have no GPU).  I tried using a sw rendering
context and a coupe of other circumventions but nothing really worked.

[1] early PSs in https://codereview.chromium.org/2677553003/

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Shape Detection: move the Mac service to the sandboxed GPU process

ShapeDetection in Mac uses privileged and potentially dangerous APIs
inside CoreImage and perhaps more, transitively.  This CL moves the 
shape detection service to run in the GPU process, which is sandboxed.

The reason for using the GPU process is that CoreImage APIs seem
to depend strongly and mysteriously on the GPU, as I learned while
trying to get the unittests to work [1] in our normal mac bots (which 
are virtual machines and have no GPU).  I tried using a sw rendering
context and a coupe of other circumventions but nothing really worked.

[1] early PSs in https://codereview.chromium.org/2677553003/

BUG=666143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2739163003
Cr-Commit-Position: refs/heads/master@{#456555}
Committed:
https://chromium.googlesource.com/chromium/src/+/95d98937e0fb0baf11d7c3088a99...
==========

[commit-bot: I haz the power, 2017-03-13 23:56:25]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/95d98937e0fb0baf11d7c3088a99...

[tzik, 2017-03-15 07:09:29]

A revert of this CL (patchset #1 id:20001) has been created in
https://codereview.chromium.org/2749213002/ by tzik@chromium.org.

The reason for reverting is:
fast/shapedetection/shapedetection-security-test.html seems to get flaky due to
this CL on mac bots.

BUG=701675
.