Change the NaCl loader and broker processes to use the ServiceManager.

Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org

Committed: https://crrev.com/2b0375b64246a09f6df36b55274138f9748741b7
Cr-Commit-Position: refs/heads/master@{#440299}

[Sam McNally, 2016-11-15 03:18:43]

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

[commit-bot: I haz the power, 2016-11-15 03:19:24]

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, 2016-11-15 03:33:29]

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

[commit-bot: I haz the power, 2016-11-15 03:33:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sam McNally, 2016-11-15 03:35:58]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2016-11-15 03:40:43]

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

[commit-bot: I haz the power, 2016-11-15 03:41:24]

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, 2016-11-15 04:14:54]

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

[commit-bot: I haz the power, 2016-11-15 04:14:55]

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

[Sam McNally, 2016-11-15 04:31:08]

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

[commit-bot: I haz the power, 2016-11-15 04:32:08]

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, 2016-11-15 04:41:39]

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

[commit-bot: I haz the power, 2016-11-15 04:41:39]

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

[Sam McNally, 2016-11-15 06:31:27]

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

[commit-bot: I haz the power, 2016-11-15 06:31:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-15 06:37:38]

Description was changed from

==========
Change the NaCl loader and broker processes to be services.
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.
==========

[commit-bot: I haz the power, 2016-11-15 06:50:19]

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

[commit-bot: I haz the power, 2016-11-15 06:50:20]

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

[Sam McNally, 2016-11-17 07:05:50]

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

[commit-bot: I haz the power, 2016-11-17 07:06:15]

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, 2016-11-17 08:41:24]

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

[commit-bot: I haz the power, 2016-11-17 08:41:24]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sam McNally, 2016-11-18 02:01:15]

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

[commit-bot: I haz the power, 2016-11-18 02:04:13]

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, 2016-11-18 02:16:52]

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

[commit-bot: I haz the power, 2016-11-18 02:16:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Sam McNally, 2016-11-18 03:06:08]

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

[commit-bot: I haz the power, 2016-11-18 03:08:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-18 04:32:30]

Description was changed from

==========
Change the NaCl loader and broker processes to use the ServiceManager.
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.

BUG=666605
==========

[Sam McNally, 2016-11-18 04:33:25]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-11-18 04:33:28]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-11-18 04:33:29]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-11-18 04:33:31]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-11-18 04:33:34]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-11-18 04:33:37]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-11-18 04:40:29]

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

[commit-bot: I haz the power, 2016-11-18 04:41:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-18 04:59:41]

Patchset #1 (id:140001) has been deleted

[Sam McNally, 2016-11-18 05:06:58]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org

[commit-bot: I haz the power, 2016-11-18 06:42:36]

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

[commit-bot: I haz the power, 2016-11-18 06:42:37]

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

[Sam McNally, 2016-11-18 07:41:56]

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

[commit-bot: I haz the power, 2016-11-18 07:42:13]

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, 2016-11-18 08:08:19]

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

[commit-bot: I haz the power, 2016-11-18 08:08:20]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-11-22 23:13:55]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2016-11-23 00:43:04]

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

[Sam McNally, 2016-11-23 00:43:30]

ping

[commit-bot: I haz the power, 2016-11-23 00:43:39]

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, 2016-11-23 02:56:11]

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

[commit-bot: I haz the power, 2016-11-23 02:56:12]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-11-23 03:27:50]

Apologies for the delay - LGTM!

[Sam McNally, 2016-11-23 04:11:03]

sammc@chromium.org changed reviewers:
+ mseaborn@chromium.org, piman@chromium.org

[Sam McNally, 2016-11-23 04:11:03]

+mseaborn for //components/nacl

+piman for //content

[piman, 2016-11-23 18:08:11]

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:258:
base::debug::Alias(&manifest.first);
Why this?

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:259:
CHECK(!contents.empty());
nit: DCHECK

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:263:
base::debug::Alias(&contents);
Why this?

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:264:
CHECK(manifest_value);
nit: DCHECK

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:267:
std::move(manifest_value));
This block looks like a bunch of copy/paste from the block above. This function
is getting long, is there a way we could refactor these 2 into a separate
function?

https://codereview.chromium.org/2501913002/diff/180001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2501913002/diff/180001/content/public/browser...
content/public/browser/content_browser_client.h:710: virtual
std::vector<std::pair<std::string, int>> GetExtraServiceManifests();
nit: what are the members of the pair? Could you change the std::pair into an
explicit struct to give them proper names (as opposed to "first" and "second"
which aren't particularly descriptive at the call sites).

Separately, please document this new function.

[Sam McNally, 2016-11-24 01:53:34]

Patchset #2 (id:200001) has been deleted

[Sam McNally, 2016-11-24 02:04:50]

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

[commit-bot: I haz the power, 2016-11-24 02:05:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-24 02:10:41]

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:258:
base::debug::Alias(&manifest.first);
On 2016/11/23 18:08:10, piman wrote:
> Why this?

Copy-pasta. Removed it.

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:259:
CHECK(!contents.empty());
On 2016/11/23 18:08:11, piman wrote:
> nit: DCHECK

I've refactored this duplicate code out, keeping the CHECK. Given
https://bugs.chromium.org/p/chromium/issues/detail?id=653382 I'm inclined to
leave them as CHECKs in this CL.

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:263:
base::debug::Alias(&contents);
On 2016/11/23 18:08:11, piman wrote:
> Why this?

Copy-pasta.

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:264:
CHECK(manifest_value);
On 2016/11/23 18:08:11, piman wrote:
> nit: DCHECK

Same as with the above CHECK.

https://codereview.chromium.org/2501913002/diff/180001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:267:
std::move(manifest_value));
On 2016/11/23 18:08:11, piman wrote:
> This block looks like a bunch of copy/paste from the block above. This
function
> is getting long, is there a way we could refactor these 2 into a separate
> function?

Done.

https://codereview.chromium.org/2501913002/diff/180001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2501913002/diff/180001/content/public/browser...
content/public/browser/content_browser_client.h:710: virtual
std::vector<std::pair<std::string, int>> GetExtraServiceManifests();
On 2016/11/23 18:08:11, piman wrote:
> nit: what are the members of the pair? Could you change the std::pair into an
> explicit struct to give them proper names (as opposed to "first" and "second"
> which aren't particularly descriptive at the call sites).
> 
> Separately, please document this new function.

Done.

[commit-bot: I haz the power, 2016-11-24 04:06:50]

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

[commit-bot: I haz the power, 2016-11-24 04:06:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[piman, 2016-11-28 23:04:37]

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:123:
CHECK(!contents.empty());
If these are to investigate a bug, please document which (open) bug it is, so
that they can be reverted to DCHECKs (and debug::Alias removed) once the bug is
fixed, as per
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
(these CHECKs should only be temporary).

crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think there's nothing
tracking removal of these CHECKs. I don't know if there's another bug for it. If
not, I'd suggest just removing the CHECKs.

[Sam McNally, 2016-11-28 23:33:22]

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:123:
CHECK(!contents.empty());
On 2016/11/28 23:04:37, piman wrote:
> If these are to investigate a bug, please document which (open) bug it is, so
> that they can be reverted to DCHECKs (and debug::Alias removed) once the bug
is
> fixed, as per
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> (these CHECKs should only be temporary).
> 
> crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think there's nothing
> tracking removal of these CHECKs. I don't know if there's another bug for it.
If
> not, I'd suggest just removing the CHECKs.

Ken: thoughts?

[Ken Rockot(use gerrit already), 2016-11-28 23:47:22]

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:123:
CHECK(!contents.empty());
On 2016/11/28 at 23:33:22, Sam McNally wrote:
> On 2016/11/28 23:04:37, piman wrote:
> > If these are to investigate a bug, please document which (open) bug it is,
so
> > that they can be reverted to DCHECKs (and debug::Alias removed) once the bug
is
> > fixed, as per
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > (these CHECKs should only be temporary).
> > 
> > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think there's
nothing
> > tracking removal of these CHECKs. I don't know if there's another bug for
it. If
> > not, I'd suggest just removing the CHECKs.
> 
> Ken: thoughts?

I'd rather just leave the CHECKs in. They aren't needed to track the bug, but
they do serve as a chokepoint for crash reports which are ultimately the result
of manifest resource corruption.

[Ken Rockot(use gerrit already), 2016-11-28 23:47:46]

On 2016/11/28 at 23:47:22, Ken Rockot wrote:
>
https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
> File content/browser/service_manager/service_manager_context.cc (right):
> 
>
https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
> content/browser/service_manager/service_manager_context.cc:123:
CHECK(!contents.empty());
> On 2016/11/28 at 23:33:22, Sam McNally wrote:
> > On 2016/11/28 23:04:37, piman wrote:
> > > If these are to investigate a bug, please document which (open) bug it is,
so
> > > that they can be reverted to DCHECKs (and debug::Alias removed) once the
bug is
> > > fixed, as per
> > >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > > (these CHECKs should only be temporary).
> > > 
> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think there's
nothing
> > > tracking removal of these CHECKs. I don't know if there's another bug for
it. If
> > > not, I'd suggest just removing the CHECKs.
> > 
> > Ken: thoughts?
> 
> I'd rather just leave the CHECKs in. They aren't needed to track the bug, but
they do serve as a chokepoint for crash reports which are ultimately the result
of manifest resource corruption.

Though I think it's fair to add a comment and reference the closed bug.

[piman, 2016-11-29 00:27:07]

On Mon, Nov 28, 2016 at 3:47 PM, <rockot@chromium.org> wrote:

>
> https://codereview.chromium.org/2501913002/diff/240001/
> content/browser/service_manager/service_manager_context.cc
> File content/browser/service_manager/service_manager_context.cc (right):
>
> https://codereview.chromium.org/2501913002/diff/240001/
> content/browser/service_manager/service_manager_context.cc#newcode123
> content/browser/service_manager/service_manager_context.cc:123:
> CHECK(!contents.empty());
> On 2016/11/28 at 23:33:22, Sam McNally wrote:
> > On 2016/11/28 23:04:37, piman wrote:
> > > If these are to investigate a bug, please document which (open) bug
> it is, so
> > > that they can be reverted to DCHECKs (and debug::Alias removed) once
> the bug is
> > > fixed, as per
> > >
> https://chromium.googlesource.com/chromium/src/+/master/
> styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
> > > (these CHECKs should only be temporary).
> > >
> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think
> there's nothing
> > > tracking removal of these CHECKs. I don't know if there's another
> bug for it. If
> > > not, I'd suggest just removing the CHECKs.
> >
> > Ken: thoughts?
>
> I'd rather just leave the CHECKs in. They aren't needed to track the
> bug, but they do serve as a chokepoint for crash reports which are
> ultimately the result of manifest resource corruption.
>

So, this is kind of bad for 2 reasons:
1- CHECK is pretty bad for end-users. They don't get any meaningful
feedback except chrome crashed. If this really happens a lot to users we
should find a better way to surface it in the UI.
2- it goes against the style guide... Bloats up the code, etc. Does
anything really warrant the exception? Once crash stacks are known and
associated with the bug, there's no real ongoing cost...

At the very least, we should certainly remove the debug::Alias, the second
CHECK (it doesn't look like it triggers), and add comments?


> https://codereview.chromium.org/2501913002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-11-29 00:38:47]

On Mon, Nov 28, 2016 at 4:26 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Nov 28, 2016 at 3:47 PM, <rockot@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2501913002/diff/240001/conte
>> nt/browser/service_manager/service_manager_context.cc
>> File content/browser/service_manager/service_manager_context.cc (right):
>>
>> https://codereview.chromium.org/2501913002/diff/240001/conte
>> nt/browser/service_manager/service_manager_context.cc#newcode123
>> content/browser/service_manager/service_manager_context.cc:123:
>> CHECK(!contents.empty());
>> On 2016/11/28 at 23:33:22, Sam McNally wrote:
>> > On 2016/11/28 23:04:37, piman wrote:
>> > > If these are to investigate a bug, please document which (open) bug
>> it is, so
>> > > that they can be reverted to DCHECKs (and debug::Alias removed) once
>> the bug is
>> > > fixed, as per
>> > >
>> https://chromium.googlesource.com/chromium/src/+/master/styl
>> eguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
>> > > (these CHECKs should only be temporary).
>> > >
>> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think
>> there's nothing
>> > > tracking removal of these CHECKs. I don't know if there's another
>> bug for it. If
>> > > not, I'd suggest just removing the CHECKs.
>> >
>> > Ken: thoughts?
>>
>> I'd rather just leave the CHECKs in. They aren't needed to track the
>> bug, but they do serve as a chokepoint for crash reports which are
>> ultimately the result of manifest resource corruption.
>>
>
> So, this is kind of bad for 2 reasons:
> 1- CHECK is pretty bad for end-users. They don't get any meaningful
> feedback except chrome crashed. If this really happens a lot to users we
> should find a better way to surface it in the UI.
>

It doesn't happen often to end-users at all. It's essentially a symptom of
malware or other tampering. Or I guess in a marginal number of cases, maybe
actual disk corruption. (a vast majority of the crash reports were INFECTED
iirc).

2- it goes against the style guide... Bloats up the code, etc. Does
> anything really warrant the exception? Once crash stacks are known and
> associated with the bug, there's no real ongoing cost...
>

I'm not sure how this goes against the style guide. With the CHECK removed
the net result is will still be an eventual crash, but the crash will
happen some time later, in a code path that may be triggered by other
actual bugs. This prevents those actionable crash reports from being
conflated with these unactionable ones. Is that not sufficient rationale?


> At the very least, we should certainly remove the debug::Alias, the second
> CHECK (it doesn't look like it triggers), and add comments?
>
>
Agreed we should remove the aliasing.


>
>> https://codereview.chromium.org/2501913002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-11-29 00:47:49]

On Mon, Nov 28, 2016 at 4:38 PM, Ken Rockot <rockot@chromium.org> wrote:

>
>
> On Mon, Nov 28, 2016 at 4:26 PM, Antoine Labour <piman@chromium.org>
> wrote:
>
>>
>>
>> On Mon, Nov 28, 2016 at 3:47 PM, <rockot@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/2501913002/diff/240001/conte
>>> nt/browser/service_manager/service_manager_context.cc
>>> File content/browser/service_manager/service_manager_context.cc (right):
>>>
>>> https://codereview.chromium.org/2501913002/diff/240001/conte
>>> nt/browser/service_manager/service_manager_context.cc#newcode123
>>> content/browser/service_manager/service_manager_context.cc:123:
>>> CHECK(!contents.empty());
>>> On 2016/11/28 at 23:33:22, Sam McNally wrote:
>>> > On 2016/11/28 23:04:37, piman wrote:
>>> > > If these are to investigate a bug, please document which (open) bug
>>> it is, so
>>> > > that they can be reverted to DCHECKs (and debug::Alias removed) once
>>> the bug is
>>> > > fixed, as per
>>> > >
>>> https://chromium.googlesource.com/chromium/src/+/master/styl
>>> eguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
>>> > > (these CHECKs should only be temporary).
>>> > >
>>> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think
>>> there's nothing
>>> > > tracking removal of these CHECKs. I don't know if there's another
>>> bug for it. If
>>> > > not, I'd suggest just removing the CHECKs.
>>> >
>>> > Ken: thoughts?
>>>
>>> I'd rather just leave the CHECKs in. They aren't needed to track the
>>> bug, but they do serve as a chokepoint for crash reports which are
>>> ultimately the result of manifest resource corruption.
>>>
>>
>> So, this is kind of bad for 2 reasons:
>> 1- CHECK is pretty bad for end-users. They don't get any meaningful
>> feedback except chrome crashed. If this really happens a lot to users we
>> should find a better way to surface it in the UI.
>>
>
> It doesn't happen often to end-users at all. It's essentially a symptom of
> malware or other tampering. Or I guess in a marginal number of cases, maybe
> actual disk corruption. (a vast majority of the crash reports were INFECTED
> iirc).
>
> 2- it goes against the style guide... Bloats up the code, etc. Does
>> anything really warrant the exception? Once crash stacks are known and
>> associated with the bug, there's no real ongoing cost...
>>
>
> I'm not sure how this goes against the style guide.
>


https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

There are 2 valid use cases for CHECK:
- preventing security vulnerabilities. It doesn't sound like this is the
case here.
- *temporarily* to investigate crashes. In this case it sounds like the
investigation is done.


> With the CHECK removed the net result is will still be an eventual crash,
> but the crash will happen some time later, in a code path that may be
> triggered by other actual bugs. This prevents those actionable crash
> reports from being conflated with these unactionable ones. Is that not
> sufficient rationale?
>

Are these actionable though? It doesn't sound like we're going to fix them
(bug is WontFix).
Are we punishing our users (larger code size, etc. if we do this at scale)
for at best a marginal utility to us developers.

If you want to change the style guide to allow this new use case, I suggest
making the case for it on chromium-dev.


>
>> At the very least, we should certainly remove the debug::Alias, the
>> second CHECK (it doesn't look like it triggers), and add comments?
>>
>>
> Agreed we should remove the aliasing.
>
>
>>
>>> https://codereview.chromium.org/2501913002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-11-29 01:07:31]

On Mon, Nov 28, 2016 at 4:47 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Nov 28, 2016 at 4:38 PM, Ken Rockot <rockot@chromium.org> wrote:
>
>>
>>
>> On Mon, Nov 28, 2016 at 4:26 PM, Antoine Labour <piman@chromium.org>
>> wrote:
>>
>>>
>>>
>>> On Mon, Nov 28, 2016 at 3:47 PM, <rockot@chromium.org> wrote:
>>>
>>>>
>>>> https://codereview.chromium.org/2501913002/diff/240001/conte
>>>> nt/browser/service_manager/service_manager_context.cc
>>>> File content/browser/service_manager/service_manager_context.cc
>>>> (right):
>>>>
>>>> https://codereview.chromium.org/2501913002/diff/240001/conte
>>>> nt/browser/service_manager/service_manager_context.cc#newcode123
>>>> content/browser/service_manager/service_manager_context.cc:123:
>>>> CHECK(!contents.empty());
>>>> On 2016/11/28 at 23:33:22, Sam McNally wrote:
>>>> > On 2016/11/28 23:04:37, piman wrote:
>>>> > > If these are to investigate a bug, please document which (open) bug
>>>> it is, so
>>>> > > that they can be reverted to DCHECKs (and debug::Alias removed) once
>>>> the bug is
>>>> > > fixed, as per
>>>> > >
>>>> https://chromium.googlesource.com/chromium/src/+/master/styl
>>>> eguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
>>>> > > (these CHECKs should only be temporary).
>>>> > >
>>>> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think
>>>> there's nothing
>>>> > > tracking removal of these CHECKs. I don't know if there's another
>>>> bug for it. If
>>>> > > not, I'd suggest just removing the CHECKs.
>>>> >
>>>> > Ken: thoughts?
>>>>
>>>> I'd rather just leave the CHECKs in. They aren't needed to track the
>>>> bug, but they do serve as a chokepoint for crash reports which are
>>>> ultimately the result of manifest resource corruption.
>>>>
>>>
>>> So, this is kind of bad for 2 reasons:
>>> 1- CHECK is pretty bad for end-users. They don't get any meaningful
>>> feedback except chrome crashed. If this really happens a lot to users we
>>> should find a better way to surface it in the UI.
>>>
>>
>> It doesn't happen often to end-users at all. It's essentially a symptom
>> of malware or other tampering. Or I guess in a marginal number of cases,
>> maybe actual disk corruption. (a vast majority of the crash reports were
>> INFECTED iirc).
>>
>> 2- it goes against the style guide... Bloats up the code, etc. Does
>>> anything really warrant the exception? Once crash stacks are known and
>>> associated with the bug, there's no real ongoing cost...
>>>
>>
>> I'm not sure how this goes against the style guide.
>>
>
>
> https://chromium.googlesource.com/chromium/src/+/master/
> styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED
>
> There are 2 valid use cases for CHECK:
> - preventing security vulnerabilities. It doesn't sound like this is the
> case here.
> - *temporarily* to investigate crashes. In this case it sounds like the
> investigation is done.
>
>
>> With the CHECK removed the net result is will still be an eventual crash,
>> but the crash will happen some time later, in a code path that may be
>> triggered by other actual bugs. This prevents those actionable crash
>> reports from being conflated with these unactionable ones. Is that not
>> sufficient rationale?
>>
>
> Are these actionable though? It doesn't sound like we're going to fix them
> (bug is WontFix).
>

These aren't actionable. The point is that if/when an actionable bug does
arise, it can lead to crashes which are conflated with these inactionable
ones, because the negative impact of missing a builtin manifest may be
deferred an arbitrarily long time, affecting various other parts of the
system.

There are examples LOG(FATAL) being used to log events which, despite being
of questionable security risk, essentially guarantee that the browser would
otherwise be unusable. Perhaps these are also wrong, but I'm not sure
crashing is a worse user experience than continuing to run a process which
is essentially in an unspecified state.

In any case I don't think this instance is worth fighting for, so fine,
let's remove them.

Are we punishing our users (larger code size, etc. if we do this at scale)
> for at best a marginal utility to us developers.
>
> If you want to change the style guide to allow this new use case, I
> suggest making the case for it on chromium-dev.
>

>
>>
>>> At the very least, we should certainly remove the debug::Alias, the
>>> second CHECK (it doesn't look like it triggers), and add comments?
>>>
>>>
>> Agreed we should remove the aliasing.
>>
>>
>>>
>>>> https://codereview.chromium.org/2501913002/
>>>>
>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sam McNally, 2016-11-29 03:26:17]

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

[commit-bot: I haz the power, 2016-11-29 03:26:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-29 03:30:14]

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/240001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:123:
CHECK(!contents.empty());
On 2016/11/28 23:47:22, Ken Rockot wrote:
> On 2016/11/28 at 23:33:22, Sam McNally wrote:
> > On 2016/11/28 23:04:37, piman wrote:
> > > If these are to investigate a bug, please document which (open) bug it is,
> so
> > > that they can be reverted to DCHECKs (and debug::Alias removed) once the
bug
> is
> > > fixed, as per
> > >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > > (these CHECKs should only be temporary).
> > > 
> > > crbug.com/653382 was closed (WontFix) 3 weeks ago, so I think there's
> nothing
> > > tracking removal of these CHECKs. I don't know if there's another bug for
> it. If
> > > not, I'd suggest just removing the CHECKs.
> > 
> > Ken: thoughts?
> 
> I'd rather just leave the CHECKs in. They aren't needed to track the bug, but
> they do serve as a chokepoint for crash reports which are ultimately the
result
> of manifest resource corruption.

Changed to DCHECKs.

[commit-bot: I haz the power, 2016-11-29 04:44:36]

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

[commit-bot: I haz the power, 2016-11-29 04:44:37]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-29 18:46:41]

lgtm

[Sam McNally, 2016-11-30 10:02:44]

mseaborn: ping

[Ken Rockot(use gerrit already), 2016-12-16 00:12:46]

rockot@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Ken Rockot(use gerrit already), 2016-12-16 00:12:47]

Friendly ping. Desperately seeking a NaCl owner to review this CL!

[bradnelson, 2016-12-16 19:00:15]

lgtm

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
File components/nacl/loader/BUILD.gn (right):

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
components/nacl/loader/BUILD.gn:199: # TODO(brettw) can this just depend on
//components/nacl/common?
Think you can drop the TODO too.

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:225:
std::unique_ptr<service_manager::ServiceContext> service_context =
service_context unused (does this need to stay alive through the init?)

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
components/nacl/loader/nonsfi/nonsfi_listener.cc:52:
std::unique_ptr<service_manager::ServiceContext> service_context =
Any reason this is kept? (unused)

[Ken Rockot(use gerrit already), 2016-12-16 19:19:47]

ServiceContext owns and maintains the connection to the service manager

[bradnelson, 2016-12-16 19:30:23]

Ah, I see.
lgtm (except for the nit about the TODO)

[Sam McNally, 2016-12-18 22:44:44]

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

[commit-bot: I haz the power, 2016-12-18 22:44:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-18 23:06:36]

sammc@chromium.org changed reviewers:
+ calamity@chromium.org, dcheng@chromium.org

[Sam McNally, 2016-12-18 23:06:37]

+calamity for chrome/browser/browser_resources.grd

+dcheng for the manifests

[Sam McNally, 2016-12-18 23:16:41]

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

[commit-bot: I haz the power, 2016-12-18 23:16:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-18 23:16:54]

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
File components/nacl/loader/BUILD.gn (right):

https://codereview.chromium.org/2501913002/diff/280001/components/nacl/loader...
components/nacl/loader/BUILD.gn:199: # TODO(brettw) can this just depend on
//components/nacl/common?
On 2016/12/16 19:00:15, bradnelson wrote:
> Think you can drop the TODO too.

Done.

[calamity, 2016-12-19 00:29:37]

browser_resources.grd lgtm.

[commit-bot: I haz the power, 2016-12-19 00:33:14]

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

[commit-bot: I haz the power, 2016-12-19 00:33:16]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-19 05:03:25]

lgtm

https://codereview.chromium.org/2501913002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2501913002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3136:
{nacl::kNaClBrokerServiceName, IDR_NACL_BROKER_MANIFEST},
If this is something clang-format is doing, do you mind filing a bug somewhere
[1] and CCing me?

[1]
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md#...

https://codereview.chromium.org/2501913002/diff/320001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/320001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:116: void
AddServiceManifest(std::string name, int resource_id) {
If you're feeling ambitious, StringPiece is probably slightly better here (since
a number of inputs come from const char arrays)

[Sam McNally, 2016-12-19 09:26:30]

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

[commit-bot: I haz the power, 2016-12-19 09:26:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 09:36:17]

sammc@chromium.org changed reviewers:
+ kmarshall@chromium.org, skyostil@chromium.org

[Sam McNally, 2016-12-19 09:36:18]

+kmarshall for //blimp

+skyostil for //headless

https://codereview.chromium.org/2501913002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2501913002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3136:
{nacl::kNaClBrokerServiceName, IDR_NACL_BROKER_MANIFEST},
On 2016/12/19 05:03:25, dcheng wrote:
> If this is something clang-format is doing, do you mind filing a bug somewhere
> [1] and CCing me?
> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md#...

https://bugs.chromium.org/p/chromium/issues/detail?id=675554

https://codereview.chromium.org/2501913002/diff/320001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2501913002/diff/320001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:116: void
AddServiceManifest(std::string name, int resource_id) {
On 2016/12/19 05:03:25, dcheng wrote:
> If you're feeling ambitious, StringPiece is probably slightly better here
(since
> a number of inputs come from const char arrays)

Done.

[commit-bot: I haz the power, 2016-12-19 09:43:40]

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

[commit-bot: I haz the power, 2016-12-19 09:43:42]

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

[Sam McNally, 2016-12-19 09:48:41]

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

[commit-bot: I haz the power, 2016-12-19 09:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 09:51:07]

sammc@chromium.org changed reviewers:
+ boliu@chromium.org, halliwell@chromium.org

[Sam McNally, 2016-12-19 09:51:08]

+boliu for //android_webview

+halliwell for //chromecast

[Sami, 2016-12-19 10:23:25]

headless/ lgtm -- thank you!

[commit-bot: I haz the power, 2016-12-19 10:43:48]

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

[commit-bot: I haz the power, 2016-12-19 10:43:50]

Dry run: This issue passed the CQ dry run.

[halliwell, 2016-12-19 14:09:03]

On 2016/12/19 10:23:25, Sami wrote:
> headless/ lgtm -- thank you!

chromecast/ lgtm

[boliu, 2016-12-19 17:21:04]

rs lgtm

[Mark Seaborn, 2016-12-19 19:17:05]

https://codereview.chromium.org/2501913002/diff/360001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/2501913002/diff/360001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.h:161: base::StringPiece
service_name) override;
Could you explain in the commit message why this change involves converting
these other parts of the codebase to use StringPiece?  (Would it be better to do
this conversion as a separate change?)

[Sam McNally, 2016-12-19 22:44:54]

Description was changed from

==========
Change the NaCl loader and broker processes to use the ServiceManager.

BUG=666605
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
==========

[Sam McNally, 2016-12-19 22:49:32]

Patchset #10 (id:380001) has been deleted

[Sam McNally, 2016-12-19 22:58:11]

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

[commit-bot: I haz the power, 2016-12-19 22:58:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 23:05:22]

https://codereview.chromium.org/2501913002/diff/360001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/2501913002/diff/360001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.h:161: base::StringPiece
service_name) override;
On 2016/12/19 19:17:05, Mark Seaborn wrote:
> Could you explain in the commit message why this change involves converting
> these other parts of the codebase to use StringPiece?  (Would it be better to
do
> this conversion as a separate change?)

Done.

[commit-bot: I haz the power, 2016-12-20 00:33:10]

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

[commit-bot: I haz the power, 2016-12-20 00:33:11]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-12-20 23:16:14]

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

[commit-bot: I haz the power, 2016-12-20 23:16:59]

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, 2016-12-21 02:06:18]

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

[commit-bot: I haz the power, 2016-12-21 02:06:20]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-12-21 06:53:13]

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

[commit-bot: I haz the power, 2016-12-21 06:53:29]

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, 2016-12-21 07:52:03]

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

[commit-bot: I haz the power, 2016-12-21 07:52:04]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-12-21 23:35:31]

Description was changed from

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org
==========

[Sam McNally, 2016-12-21 23:36:13]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-12-21 23:36:14]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
bradnelson@chromium.org, piman@chromium.org, calamity@chromium.org,
dcheng@chromium.org, skyostil@chromium.org, boliu@chromium.org,
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/2501913002/#ps440001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-21 23:36:44]

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, 2016-12-21 23:46:35]

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

[commit-bot: I haz the power, 2016-12-21 23:46:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sam McNally, 2016-12-22 00:12:34]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-12-22 00:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
calamity@chromium.org, skyostil@chromium.org, bradnelson@chromium.org,
boliu@chromium.org, dcheng@chromium.org, piman@chromium.org,
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/2501913002/#ps460001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-22 00:13:06]

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, 2016-12-22 01:26:21]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1482365554470700,
"parent_rev": "ba868ffc64ed1eda2dc069147ca6ce12a7f53861", "commit_rev":
"c1a0083954f0ba6b062d73846726eef0830f5208"}

[commit-bot: I haz the power, 2016-12-22 01:26:52]

Description was changed from

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org

Review-Url: https://codereview.chromium.org/2501913002
==========

[commit-bot: I haz the power, 2016-12-22 01:26:54]

Committed patchset #13 (id:460001)

[commit-bot: I haz the power, 2016-12-22 01:29:48]

Description was changed from

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org

Review-Url: https://codereview.chromium.org/2501913002
==========

to

==========
Change the NaCl loader and broker processes to use the ServiceManager.

This is the first child process outside of content to be connected to
the ServiceManager so this CL adds a way for content embedders to
provide additional service manifests via ContentBrowserClient. This
would introduce an unnecessary string copy when obtaining service
manifest overlays, so this changes that API to take a StringPiece.

BUG=666605
TBR=kmarshall@chromium.org

Committed: https://crrev.com/2b0375b64246a09f6df36b55274138f9748741b7
Cr-Commit-Position: refs/heads/master@{#440299}
==========

[commit-bot: I haz the power, 2016-12-22 01:29:50]

Patchset 13 (id:??) landed as
https://crrev.com/2b0375b64246a09f6df36b55274138f9748741b7
Cr-Commit-Position: refs/heads/master@{#440299}