Call SetApplicationIsDaemon() in V2 sandbox.

Call SetApplicationIsDaemon() in V2 sandbox.

Calls SetApplicationIsDaemon() to prevent crashing if LaunchServices
cannot be connected to. This CL also allows the com.apple.lsdb.mapdb
service which exposes the LaunchServices database.

BUG=689306
Review-Url: https://codereview.chromium.org/2944623003
Cr-Commit-Position: refs/heads/master@{#485092}
Committed: https://chromium.googlesource.com/chromium/src/+/2b5417ac028acd75faefeabd7102c6ebc37772f2

[Greg K, 2017-06-16 18:50:14]

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

[Greg K, 2017-06-16 18:50:15]

PTAL.

Thanks,

Greg

[Robert Sesek, 2017-06-16 19:28:59]

https://codereview.chromium.org/2944623003/diff/20001/content/common/sandbox_...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/20001/content/common/sandbox_...
content/common/sandbox_mac.mm:322: SetApplicationIsDaemon(true);
I think this call should just move to
https://cs.chromium.org/chromium/src/content/renderer/renderer_main_platform_....

[Greg K, 2017-06-16 20:31:02]

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

[commit-bot: I haz the power, 2017-06-16 20:31:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Greg K, 2017-06-16 20:32:12]

https://codereview.chromium.org/2944623003/diff/20001/content/common/sandbox_...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/20001/content/common/sandbox_...
content/common/sandbox_mac.mm:322: SetApplicationIsDaemon(true);
On 2017/06/16 19:28:59, Robert Sesek wrote:
> I think this call should just move to
>
https://cs.chromium.org/chromium/src/content/renderer/renderer_main_platform_....

Done.

[Robert Sesek, 2017-06-16 20:40:44]

LGTM

[commit-bot: I haz the power, 2017-06-16 23:12:43]

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

[commit-bot: I haz the power, 2017-06-16 23:12:44]

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

[Greg K, 2017-06-27 22:37:49]

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

[commit-bot: I haz the power, 2017-06-27 22:38: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-06-28 01:17:07]

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

[commit-bot: I haz the power, 2017-06-28 01:17:08]

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

[Greg K, 2017-06-28 17:19:29]

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

[commit-bot: I haz the power, 2017-06-28 17:19: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-06-28 20:04:30]

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

[commit-bot: I haz the power, 2017-06-28 20:04:31]

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

[Greg K, 2017-06-28 20:16:20]

kerrnel@chromium.org changed reviewers:
+ avi@chromium.org

[Greg K, 2017-06-28 20:16:20]

avi@chromium.org: Please review changes in 

PTAL at content/renderer

Thanks,

Greg

[Avi (use Gerrit), 2017-06-28 21:03:02]

lgtm

[Greg K, 2017-06-28 21:21:04]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2017-06-28 21:21:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2944623003/#ps60001
(title: "Call SetApplicationIsDaemon() in V2 sandbox.")

[commit-bot: I haz the power, 2017-06-28 21:21:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Robert Sesek, 2017-06-28 21:33:26]

The CQ bit was unchecked by rsesek@chromium.org

[Robert Sesek, 2017-06-28 21:33:57]

Failures on the trybots look legit. Did you test this locally? Maybe it's a 10.9
issue only (trybots run 10.9).

[Greg K, 2017-06-28 21:42:14]

On 2017/06/28 21:33:57, Robert Sesek wrote:
> Failures on the trybots look legit. Did you test this locally? Maybe it's a
10.9
> issue only (trybots run 10.9).

Ahh, I was going to let the trybot run one more time and let it land if it
passed, but does it ignore trybots that are purple? Anyhow I'll run in my 10.9
VM.

Thanks,

Greg

[Robert Sesek, 2017-06-28 21:43:26]

On 2017/06/28 21:42:14, Greg K wrote:
> On 2017/06/28 21:33:57, Robert Sesek wrote:
> > Failures on the trybots look legit. Did you test this locally? Maybe it's a
> 10.9
> > issue only (trybots run 10.9).
> 
> Ahh, I was going to let the trybot run one more time and let it land if it
> passed, but does it ignore trybots that are purple? Anyhow I'll run in my 10.9
> VM.
> 
> Thanks,
> 
> Greg

Check the stdout:

_RegisterApplication(), unable to get application ASN from launchservicesd, and
this application requires an ASN, so aborting.  error=#-1.
Received signal 6
 [0x000102ea1cdc]
 [0x000102ea1b91]
 [0x7fff90ed05aa]
 [0xffffffffffffffff]
 [0x7fff89944b1a]
 [0x7fff8b3357fb]
 [0x7fff8b351fc0]

[Robert Sesek, 2017-06-28 22:15:24]

https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_mac.mm:135: // Allow the
process to continue without a LaunchServices ASN. The
Maybe moving this below InitializeSandbox() will fix?

[Greg K, 2017-06-29 23:31:28]

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

[Greg K, 2017-06-29 23:31:37]

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

[commit-bot: I haz the power, 2017-06-29 23:31:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Greg K, 2017-06-30 00:46:16]

On 2017/06/28 22:15:24, Robert Sesek wrote:
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> File content/renderer/renderer_main_platform_delegate_mac.mm (right):
> 
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> content/renderer/renderer_main_platform_delegate_mac.mm:135: // Allow the
> process to continue without a LaunchServices ASN. The
> Maybe moving this below InitializeSandbox() will fix?

Ok the trybots pass now. Is this still LGTM with the additional calls to
disconnect the window server moved around?

Regards,

Greg

[commit-bot: I haz the power, 2017-06-30 00:51:29]

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

[commit-bot: I haz the power, 2017-06-30 00:51:30]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-06-30 16:06:50]

On 2017/06/30 00:46:16, Greg K wrote:
> On 2017/06/28 22:15:24, Robert Sesek wrote:
> >
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> > File content/renderer/renderer_main_platform_delegate_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> > content/renderer/renderer_main_platform_delegate_mac.mm:135: // Allow the
> > process to continue without a LaunchServices ASN. The
> > Maybe moving this below InitializeSandbox() will fix?
> 
> Ok the trybots pass now. Is this still LGTM with the additional calls to
> disconnect the window server moved around?
> 
> Regards,
> 
> Greg

This logic isn't the same, and I think there may be consequences of that.

Before:

- Sandbox warmup
- Deny and shutdown window server connection
- Set application daemon
- Enable sandbox

After:

- Deny and shutdown window server connection
- Set application daemon
- Sandbox warmup
- Enable sandbox

I worry that putting the window server deny and shutdown before sandbox warmup
will break something, since warmup calls do try and connect to the window
server. Maybe this instead?

- Sandbox warmup
- Deny and shutdown window server connection
- Enable sandbox
- Set application daemon

That would allow the window server connection manipulation to stay in
sandbox_mac.mm, and then the set application daemon call can be in
renderer_main_platform_delegate_mac.mm after InitializeSandbox() (which is what
calls SandboxWarmup).

Also, did you test this locally in a VM?

[Greg K, 2017-07-03 20:53:18]

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

[commit-bot: I haz the power, 2017-07-03 20:53:34]

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

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

[commit-bot: I haz the power, 2017-07-03 21:16:43]

Dry run: This issue passed the CQ dry run.

[Greg K, 2017-07-03 21:20:09]

On 2017/06/30 16:06:50, Robert Sesek wrote:
> On 2017/06/30 00:46:16, Greg K wrote:
> > On 2017/06/28 22:15:24, Robert Sesek wrote:
> > >
> >
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> > > File content/renderer/renderer_main_platform_delegate_mac.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2944623003/diff/60001/content/renderer/render...
> > > content/renderer/renderer_main_platform_delegate_mac.mm:135: // Allow the
> > > process to continue without a LaunchServices ASN. The
> > > Maybe moving this below InitializeSandbox() will fix?
> > 
> > Ok the trybots pass now. Is this still LGTM with the additional calls to
> > disconnect the window server moved around?
> > 
> > Regards,
> > 
> > Greg
> 
> This logic isn't the same, and I think there may be consequences of that.
> 
> Before:
> 
> - Sandbox warmup
> - Deny and shutdown window server connection
> - Set application daemon
> - Enable sandbox
> 
> After:
> 
> - Deny and shutdown window server connection
> - Set application daemon
> - Sandbox warmup
> - Enable sandbox
> 
> I worry that putting the window server deny and shutdown before sandbox warmup
> will break something, since warmup calls do try and connect to the window
> server. Maybe this instead?
> 
> - Sandbox warmup
> - Deny and shutdown window server connection
> - Enable sandbox
> - Set application daemon
> 
> That would allow the window server connection manipulation to stay in
> sandbox_mac.mm, and then the set application daemon call can be in
> renderer_main_platform_delegate_mac.mm after InitializeSandbox() (which is
what
> calls SandboxWarmup).

Right, that's a fair point so I restructured it as such.

> 
> Also, did you test this locally in a VM?

Yes, that's how I test that about:flags works, etc. There's a separate CL needed
to get the actual V2 sandbox working properly on 10.9, but I want to keep this
CL focused on this SetApplicationIsDaemon() change and making sure the
browser_tests pass on 10.9.

- Greg

[Greg K, 2017-07-03 21:26:08]

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_mac.mm:140: //
`ExplicitlyWarmupSandbox` and `ExplicitlyEnableSandbox` are no-ops
I know there are a lot of comments here but I kept getting confused myself until
I documented it so I think the comment is worthwhile.

[Robert Sesek, 2017-07-06 21:28:09]

https://codereview.chromium.org/2944623003/diff/100001/content/common/sandbox...
File content/common/sandbox_init_mac.h (right):

https://codereview.chromium.org/2944623003/diff/100001/content/common/sandbox...
content/common/sandbox_init_mac.h:21: void ExplicitlyWarmupSandbox();
I know this is probably temporary code, but this interface seems rather
error-prone. I thought of something else, though: add a new InitializeSandbox()
function that takes a base::Closure to run after warmup has completed but before
the sandbox is initialized. Something like this:

bool InitializeSandboxWithPostWarmupHook(base::OnceClosure hook);

… and then InitializeSandbox() could be implemented in terms of that with a null
callback.

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_mac.mm:136: bool
RendererMainPlatformDelegate::EnableSandbox() {
(… continuing comment from sandbox_init_mac.h):

The WindowServer calls could then move into a separate function
DisconnectWindowServer(). Then this method could simply make calling calling
InitializeSandboxWithPostWarmupHook() conditional on the v2 flag not being
present.

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_mac.mm:140: //
`ExplicitlyWarmupSandbox` and `ExplicitlyEnableSandbox` are no-ops
On 2017/07/03 21:26:08, Greg K wrote:
> I know there are a lot of comments here but I kept getting confused myself
until
> I documented it so I think the comment is worthwhile.

I think the comments are very helpful as well.

[Greg K, 2017-07-07 18:09:50]

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

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

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Greg K, 2017-07-07 18:10:21]

https://codereview.chromium.org/2944623003/diff/100001/content/common/sandbox...
File content/common/sandbox_init_mac.h (right):

https://codereview.chromium.org/2944623003/diff/100001/content/common/sandbox...
content/common/sandbox_init_mac.h:21: void ExplicitlyWarmupSandbox();
On 2017/07/06 21:28:09, Robert Sesek wrote:
> I know this is probably temporary code, but this interface seems rather
> error-prone. I thought of something else, though: add a new
InitializeSandbox()
> function that takes a base::Closure to run after warmup has completed but
before
> the sandbox is initialized. Something like this:
> 
> bool InitializeSandboxWithPostWarmupHook(base::OnceClosure hook);
> 
> … and then InitializeSandbox() could be implemented in terms of that with a
null
> callback. 

Done.

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_mac.mm:136: bool
RendererMainPlatformDelegate::EnableSandbox() {
On 2017/07/06 21:28:09, Robert Sesek wrote:
> (… continuing comment from sandbox_init_mac.h):
> 
> The WindowServer calls could then move into a separate function
> DisconnectWindowServer(). Then this method could simply make calling calling
> InitializeSandboxWithPostWarmupHook() conditional on the v2 flag not being
> present.

Done.

https://codereview.chromium.org/2944623003/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_mac.mm:140: //
`ExplicitlyWarmupSandbox` and `ExplicitlyEnableSandbox` are no-ops
On 2017/07/06 21:28:09, Robert Sesek wrote:
> On 2017/07/03 21:26:08, Greg K wrote:
> > I know there are a lot of comments here but I kept getting confused myself
> until
> > I documented it so I think the comment is worthwhile.
> 
> I think the comments are very helpful as well.

Acknowledged.

[Robert Sesek, 2017-07-07 18:15:46]

LGTM

[commit-bot: I haz the power, 2017-07-07 20:57:44]

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

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

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

[Greg K, 2017-07-07 22:09:36]

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

[commit-bot: I haz the power, 2017-07-07 22:10:10]

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-07-07 22:41:04]

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

[commit-bot: I haz the power, 2017-07-07 22:41:06]

Dry run: This issue passed the CQ dry run.

[Greg K, 2017-07-07 23:12:42]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2017-07-07 23:12:43]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2944623003/#ps140001
(title: "Only execute callback if not-null")

[commit-bot: I haz the power, 2017-07-07 23:13:00]

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-07-07 23:19:12]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499469162476980,
"parent_rev": "0ea82f3864289d9103d288edc822f264e62ca9a4", "commit_rev":
"2b5417ac028acd75faefeabd7102c6ebc37772f2"}

[commit-bot: I haz the power, 2017-07-07 23:19:24]

Description was changed from

==========
Call SetApplicationIsDaemon() in V2 sandbox.

Calls SetApplicationIsDaemon() to prevent crashing if LaunchServices
cannot be connected to. This CL also allows the com.apple.lsdb.mapdb
service which exposes the LaunchServices database.

BUG=689306
==========

to

==========
Call SetApplicationIsDaemon() in V2 sandbox.

Calls SetApplicationIsDaemon() to prevent crashing if LaunchServices
cannot be connected to. This CL also allows the com.apple.lsdb.mapdb
service which exposes the LaunchServices database.

BUG=689306

Review-Url: https://codereview.chromium.org/2944623003
Cr-Commit-Position: refs/heads/master@{#485092}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b5417ac028acd75faefeabd7102...
==========

[commit-bot: I haz the power, 2017-07-07 23:19:25]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2b5417ac028acd75faefeabd7102...