Utility process: initialize GTK when the sandbox is disabled.

Utility process: initialize GTK when the sandbox is disabled.

BUG=309093
TEST=Open the Web Store, utility process launches correctly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230287

[Ken Russell (switch to Gerrit), 2013-10-22 01:10:37]

Thanks for fixing this. The #ifdefs match up between the old and new code as far
as I can tell. LGTM

[jln (very slow on Chromium), 2013-10-22 01:23:55]

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
File content/utility/utility_main.cc (right):

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:44: // On Linux, we only initialize GLib/GTK if
we're not sandboxed.
This is super weird-looking. The utility process really should know whether it's
started from the Zygote or not.

Could you add this information to |parameters| instead?

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:67: LinuxSandbox::InitializeSandbox();
This is ugly. You should create a "const bool is_sandboxing_desired" and use
that.

Hopefully it can just come from |parameters| as suggested above. If not you
could have one localized hack above to set that bool, which you would then use
twice.

However, with the current "fake" BPF policy, there is no point in not
initializing the sandbox earlier, but I agree it looks weird. While you're at
it, add a comment that the BPF sandbox is a TODO().

[Jorge Lucangeli Obes, 2013-10-22 16:14:54]

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
File content/utility/utility_main.cc (right):

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:44: // On Linux, we only initialize GLib/GTK if
we're not sandboxed.
On 2013/10/22 01:23:55, jln wrote:
> This is super weird-looking. The utility process really should know whether
it's
> started from the Zygote or not.
> 
> Could you add this information to |parameters| instead?

Well, we do know our cmdline flags, and we use --no-sandbox to sidestep the
Zygote.

I feel more comfortable looking at the actual state of the process rather than
whatever we got passed in |parameters|.

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:67: LinuxSandbox::InitializeSandbox();
On 2013/10/22 01:23:55, jln wrote:
> This is ugly. You should create a "const bool is_sandboxing_desired" and use
> that.
> 
> Hopefully it can just come from |parameters| as suggested above. If not you
> could have one localized hack above to set that bool, which you would then use
> twice.
> 
> However, with the current "fake" BPF policy, there is no point in not
> initializing the sandbox earlier, but I agree it looks weird. While you're at
> it, add a comment that the BPF sandbox is a TODO().

GTK starts threads, so we actually do need to know whether GTK was initialized.
We can deduce it from whether we're sandboxed, but we need to take it into
account.

[jln (very slow on Chromium), 2013-10-22 16:42:40]

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
File content/utility/utility_main.cc (right):

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:44: // On Linux, we only initialize GLib/GTK if
we're not sandboxed.
On 2013/10/22 16:14:54, Jorge Lucangeli Obes wrote:
> On 2013/10/22 01:23:55, jln wrote:
> > This is super weird-looking. The utility process really should know whether
> it's
> > started from the Zygote or not.
> > 
> > Could you add this information to |parameters| instead?
> 
> Well, we do know our cmdline flags, and we use --no-sandbox to sidestep the
> Zygote.
> 
> I feel more comfortable looking at the actual state of the process rather than
> whatever we got passed in |parameters|.

It's error prone though. You could have a DCHECK that checks that the states
matches what it should be, don't you think it's cleaner?

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:67: LinuxSandbox::InitializeSandbox();
On 2013/10/22 16:14:54, Jorge Lucangeli Obes wrote:
> On 2013/10/22 01:23:55, jln wrote:
> > This is ugly. You should create a "const bool is_sandboxing_desired" and use
> > that.
> > 
> > Hopefully it can just come from |parameters| as suggested above. If not you
> > could have one localized hack above to set that bool, which you would then
use
> > twice.
> > 
> > However, with the current "fake" BPF policy, there is no point in not
> > initializing the sandbox earlier, but I agree it looks weird. While you're
at
> > it, add a comment that the BPF sandbox is a TODO().
> 
> GTK starts threads, so we actually do need to know whether GTK was
initialized.
> We can deduce it from whether we're sandboxed, but we need to take it into
> account.

I realize, but again, there should be some "logic" that decides what we're
doing. If we start the (empty) sandbox because gtk was not started, we don't
really know that we're starting the sandbox when it matters.

In any case, for now, until a real BPF policy exists, you can
InitializeSandbox() before GTK.

[Jorge Lucangeli Obes, 2013-10-22 16:58:43]

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
File content/utility/utility_main.cc (right):

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:67: LinuxSandbox::InitializeSandbox();
On 2013/10/22 16:42:40, jln wrote:
> On 2013/10/22 16:14:54, Jorge Lucangeli Obes wrote:
> > On 2013/10/22 01:23:55, jln wrote:
> > > This is ugly. You should create a "const bool is_sandboxing_desired" and
use
> > > that.
> > > 
> > > Hopefully it can just come from |parameters| as suggested above. If not
you
> > > could have one localized hack above to set that bool, which you would then
> use
> > > twice.
> > > 
> > > However, with the current "fake" BPF policy, there is no point in not
> > > initializing the sandbox earlier, but I agree it looks weird. While you're
> at
> > > it, add a comment that the BPF sandbox is a TODO().
> > 
> > GTK starts threads, so we actually do need to know whether GTK was
> initialized.
> > We can deduce it from whether we're sandboxed, but we need to take it into
> > account.
> 
> I realize, but again, there should be some "logic" that decides what we're
> doing. If we start the (empty) sandbox because gtk was not started, we don't
> really know that we're starting the sandbox when it matters.
> 
> In any case, for now, until a real BPF policy exists, you can
> InitializeSandbox() before GTK.

I'm worried that we'll change the BPF policy and things are gonna break, but
I'll put a big fat TODO there and in sandbox_seccomp_bpf_linux.cc.

[Jorge Lucangeli Obes, 2013-10-22 17:56:14]

PTAL.

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
File content/utility/utility_main.cc (right):

https://chromiumcodereview.appspot.com/34123003/diff/1/content/utility/utilit...
content/utility/utility_main.cc:44: // On Linux, we only initialize GLib/GTK if
we're not sandboxed.
On 2013/10/22 16:42:40, jln wrote:
> On 2013/10/22 16:14:54, Jorge Lucangeli Obes wrote:
> > On 2013/10/22 01:23:55, jln wrote:
> > > This is super weird-looking. The utility process really should know
whether
> > it's
> > > started from the Zygote or not.
> > > 
> > > Could you add this information to |parameters| instead?
> > 
> > Well, we do know our cmdline flags, and we use --no-sandbox to sidestep the
> > Zygote.
> > 
> > I feel more comfortable looking at the actual state of the process rather
than
> > whatever we got passed in |parameters|.
> 
> It's error prone though. You could have a DCHECK that checks that the states
> matches what it should be, don't you think it's cleaner?

Done.

[Jorge Lucangeli Obes, 2013-10-22 19:01:18]

Small fix for non-GTK POSIX targets. PTAL.

[jln (very slow on Chromium), 2013-10-22 19:02:45]

Initializing the sandbox early means we'll need to resume the previous
discussion later, but it looks like the right thing to do to fix this bug fast.

lgtm

[Matt Perry, 2013-10-22 19:11:11]

lgtm

[commit-bot: I haz the power, 2013-10-22 20:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/34123003/220001

[commit-bot: I haz the power, 2013-10-22 22:35:13]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-22 22:49:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/34123003/220001

[commit-bot: I haz the power, 2013-10-23 02:03:19]

Change committed as 230287