Enable sandbox in local discovery utility process.

Enable sandbox in local discovery utility process.
Local discovery in sandbox works only on Windows, linux requires --no-sandbox.

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

[gene, 2013-07-18 08:55:28]

lgtm
nit below

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
chrome/utility/local_discovery/service_discovery_message_handler.cc:136: false;
return false?

[Noam Samuel, 2013-07-18 16:29:07]

https://codereview.chromium.org/19737002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/service_discovery_host_client.cc (right):

https://codereview.chromium.org/19737002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/service_discovery_host_client.cc:161:
base::Bind(&ServiceDiscoveryHostClient::ShutdownOnIOThread, this));
Is there a potential race condition here if shutdown finishes before the
ServiceDiscoveryHostClient is de-refed on the UI thread, causing the utility
process host to be destroyed on the UI thread rather than on the IO thread?

https://codereview.chromium.org/19737002/diff/1/content/browser/utility_proce...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/19737002/diff/1/content/browser/utility_proce...
content/browser/utility_process_host_impl.cc:128: EndBatchMode();
What's behind this change? Seems like so long as users of
ServiceDiscoveryProcessHost call Shutdown(), batch mode will be ended before the
utility host is destroyed.

[Vitaly Buka (NO REVIEWS), 2013-07-18 18:16:15]

https://codereview.chromium.org/19737002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/service_discovery_host_client.cc (right):

https://codereview.chromium.org/19737002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/service_discovery_host_client.cc:161:
base::Bind(&ServiceDiscoveryHostClient::ShutdownOnIOThread, this));
On 2013/07/18 16:29:07, Noam Samuel wrote:
> Is there a potential race condition here if shutdown finishes before the
> ServiceDiscoveryHostClient is de-refed on the UI thread, causing the utility
> process host to be destroyed on the UI thread rather than on the IO thread?

No.

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
chrome/utility/local_discovery/service_discovery_message_handler.cc:136: false;
On 2013/07/18 08:55:28, gene wrote:
> return false?

yes. here we have mdns_client_!=NULL and service_discovery_client_== NULL this
means we can't initialize for some reason.

https://codereview.chromium.org/19737002/diff/1/content/browser/utility_proce...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/19737002/diff/1/content/browser/utility_proce...
content/browser/utility_process_host_impl.cc:128: EndBatchMode();
On 2013/07/18 16:29:07, Noam Samuel wrote:
> What's behind this change? Seems like so long as users of
> ServiceDiscoveryProcessHost call Shutdown(), batch mode will be ended before
the
> utility host is destroyed.

ServiceDiscoveryHostClient and other host clients usually keep WeepPtr
UtilityProcessHost. When Browser shutdowns HostClient usually has dead pointer
to Host so it can't call shutdown.

Also I see no reason why Host can't just shutdown itself if it leaved in batch
mode.

[Noam Samuel, 2013-07-18 18:17:53]

lgtm

[Vitaly Buka (NO REVIEWS), 2013-07-18 18:18:38]

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
chrome/utility/local_discovery/service_discovery_message_handler.cc:136: false;
On 2013/07/18 18:16:16, Vitaly Buka wrote:
> On 2013/07/18 08:55:28, gene wrote:
> > return false?
> 
> yes. here we have mdns_client_!=NULL and service_discovery_client_== NULL this
> means we can't initialize for some reason.

I see :-)

[Vitaly Buka (NO REVIEWS), 2013-07-18 18:20:42]

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/19737002/diff/1/chrome/utility/local_discover...
chrome/utility/local_discovery/service_discovery_message_handler.cc:136: false;
On 2013/07/18 08:55:28, gene wrote:
> return false?

Done.

[Vitaly Buka (NO REVIEWS), 2013-07-18 18:47:53]

Justin, would you like to do security review for this patch. Main part is
chrome/utility/local_discovery/service_discovery_message_handler.cc

[Vitaly Buka (NO REVIEWS), 2013-07-18 19:05:48]

+darin for content/
+thestig for chrome/

[Lei Zhang, 2013-07-18 19:50:00]

https://chromiumcodereview.appspot.com/19737002/diff/14001/chrome/app/chrome_...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/19737002/diff/14001/chrome/app/chrome_...
chrome/app/chrome_main_delegate.cc:635:
chrome::ChromeContentUtilityClient::PreSandboxStartup();
Do all process types need this, or only utility processes?

https://chromiumcodereview.appspot.com/19737002/diff/14001/content/public/com...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/19737002/diff/14001/content/public/com...
content/public/common/content_switches.h:238: extern const char
kUtilityProcessEnableMDns[];
Do you need CONTENT_EXPORT since you are using it from
chrome/utility/local_discovery/service_discovery_message_handler.cc ?

[Vitaly Buka (NO REVIEWS), 2013-07-18 20:42:13]

https://chromiumcodereview.appspot.com/19737002/diff/14001/chrome/app/chrome_...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/19737002/diff/14001/chrome/app/chrome_...
chrome/app/chrome_main_delegate.cc:635:
chrome::ChromeContentUtilityClient::PreSandboxStartup();
On 2013/07/18 19:50:01, Lei Zhang wrote:
> Do all process types need this, or only utility processes?

Thanks.
Done.

https://chromiumcodereview.appspot.com/19737002/diff/14001/content/public/com...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/19737002/diff/14001/content/public/com...
content/public/common/content_switches.h:238: extern const char
kUtilityProcessEnableMDns[];
On 2013/07/18 19:50:01, Lei Zhang wrote:
> Do you need CONTENT_EXPORT since you are using it from
> chrome/utility/local_discovery/service_discovery_message_handler.cc ?

I use component=shared_library and for some reason it does not fail.
Done.

[Lei Zhang, 2013-07-18 20:46:38]

lgtm

[jschuh, 2013-07-19 17:16:12]

So, basically just starting up the network and getting your socket before the
sandbox goes into lockdown? Seems fine. lgtm

[Vitaly Buka (NO REVIEWS), 2013-07-19 21:22:53]

+creis for content

[Charlie Reis, 2013-07-19 21:34:11]

content/ LGTM, given Justin's approval for the sandbox aspect.

[commit-bot: I haz the power, 2013-07-19 22:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/19737002/28001

[commit-bot: I haz the power, 2013-07-19 23:29:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-19 23:48:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/19737002/49001

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

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-07-20 00:11:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/19737002/38002

[commit-bot: I haz the power, 2013-07-20 00:31:01]

Failed to trigger a try job on linux_clang
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-20 00:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/19737002/56002

[commit-bot: I haz the power, 2013-07-20 01:09:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-20 07:57:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/19737002/32002

[commit-bot: I haz the power, 2013-07-20 11:51:55]

Change committed as 212781