I skimmed the call-sites, and in many places the data pipe is used without checked. Here is the list (there may be false positives, sorry).

 - chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc
 - content/browser/blob_storage/blob_internals_url_loader.cc
 - content/browser/devtools/devtools_url_loader_interceptor.cc
 - content/browser/web_package/signed_exchange_loader.cc
 - content/browser/webui/web_ui_url_loader_factory.cc
 - content/renderer/service_worker/service_worker_subresource_loader.cc
 - extensions/browser/extension_protocols.cc
 - services/network/url_loader.cc

In many places mojo::DataPipe is used. The interface is very confusing I think, because there is no obvious way to detect errors, and actually the implementation says there is no error with a DCHECK.

inline DataPipe::DataPipe(uint32_t capacity_num_bytes) {
  MojoCreateDataPipeOptions options;
  options.struct_size = sizeof(MojoCreateDataPipeOptions);
  options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
  options.element_num_bytes = 1;
  options.capacity_num_bytes = capacity_num_bytes;
  mojo::DataPipe data_pipe(options);
  MojoResult result =
      CreateDataPipe(&options, &producer_handle, &consumer_handle);
  ALLOW_UNUSED_LOCAL(result);
  DCHECK_EQ(MOJO_RESULT_OK, result);
}

But I think that DCHECK is incorrect. CreateDataPipe can fail with various reasons including resource exhaustion.

Ken, is my understanding correct?

Yeah we should probably get rid of the DataPipe class altogether. It's not
a good API and the DCHECK is obviously wrong. Unlike other cases (like
message pipe creation) where resource exhaustion is extremely unlikely,
shared memory allocation failure happens fairly often. Two possible
suggestions:

a) change the DCHECK to a CHECK, because what can we even do if shared
memory allocation fails? I don't think there's a sane continuation path in
most cases?

b) get rid of the DataPipe class and just add a:

  MojoResult CreateDataPipe(/* options... */, ScopedDataPipeConsumerHandle*
consumer, ScopedDataPipeProducerHandle *producer);

API to use everywhere.


But with the latter option I still wonder what recourse callers really have
when it fails.

>#3 

Thanks, replacing DCHECKs with CHECKs is good at least for investigation. I uploaded https://chromium-review.googlesource.com/c/chromium/src/+/1258686. Let's see if our guess is correct after it's landed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86172124824f84f4f0b93129bada55664178b2cb

commit 86172124824f84f4f0b93129bada55664178b2cb
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Wed Oct 03 07:35:49 2018

Replace DCHECKs in mojo::DataPipe constructors with CHECK

There are some failures when validating URLLoaderClient method
parameters, and one suspect is null data pipe. There are some callsites
passing data pipes created with mojo::DataPipe without null checks.
There are DCHECK in DataPipe constructors, but we suspect the
assumption fails in the wild, so turn them to CHECK.

Bug:  890468 
Change-Id: I222af288ac17a439ac2cfbc4b1bd41c12cd4fb70
Reviewed-on: https://chromium-review.googlesource.com/c/1258686
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596142}
[modify] https://crrev.com/86172124824f84f4f0b93129bada55664178b2cb/mojo/public/cpp/system/data_pipe.h

hi, did we get any data back yet? what's the next step here?

There is code that uses the mojo::DataPipe() constructor and then checks the resulting handles for validity.  It seems the commit in #c5 broke all these fallible checks.  Should we go through with migrating all this code to the more verbose CreateDataPipe or is the constructor going to be put back?

Re #6: Lots of data - see issue 892297.

Reviewing the reports most of the ones under URLLoader stacks were from systems with <2000 pages (i.e. <4MB) of system commit remaining, indicating that those were OOM failures.

There are also MojoVideoDecoder and ServiceWorkerLoaderHelpers stacks, which do not seem to have low commit-limit, so not sure what might explain those.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bec6951df50f0828bc7b154376e056603b10ae9

commit 7bec6951df50f0828bc7b154376e056603b10ae9
Author: Wez <wez@chromium.org>
Date: Mon Oct 08 18:36:15 2018

Fix DataPipe(capacity) constructor not to double-allocate.

The DataPipe(capacity) constructor was previously creating a DataPipe
on the stack, using the DataPipe(options) constructor, but then not
using it, and instead manually creating the pipe handles to return to
the caller.

Remove the wasted work, and clean up some ALLOW_UNUSED_LOCAL() that are
no longer required, since DCHECKs in DataPipe() constructors were
replaced with CHECKs.

Bug: 892297,  890468 
Change-Id: Id09cd7878faa9b796858169125ff6f05d19dc5f8
Reviewed-on: https://chromium-review.googlesource.com/c/1266602
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597623}
[modify] https://crrev.com/7bec6951df50f0828bc7b154376e056603b10ae9/mojo/public/cpp/system/data_pipe.h

https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BDump+without+crash%5D+content%3A%3ABrowserChildProcessHostImpl%3A%3AOnMojoError%27+and+product.version+%3E+%2771.0.%27

It's now clear that empty handle was the cause.

I'm leaning to handling the failure as OOM, so closing this bug as fixed.

This seems to only happen with network service enabled: https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27beta%27+AND+product.Version%3D%2770.0.3538.45%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27&compProp=expanded_custom_data.ChromeCrashProto.experiments.ids&v1=e111fcd-f23d1dea&v2=e111fcd-3f4a17df

If it was an OOM, wouldn't it be the same in both cases?

Reopening until we figure out why we're killing renderers with network service enabled but not disabled.

Apologies, re-reading this again and looking at minidumps I now see that it's utility process (e.g. network process) that's OOMing, and with the commit in comment 5 that it's now gone and replaced with a precise CHECK.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e35b1ad216fa7ef04b66177d49f692976ad8bf5d

commit e35b1ad216fa7ef04b66177d49f692976ad8bf5d
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Oct 11 22:28:27 2018

Fix DataPipe(capacity) constructor not to double-allocate.

The DataPipe(capacity) constructor was previously creating a DataPipe
on the stack, using the DataPipe(options) constructor, but then not
using it, and instead manually creating the pipe handles to return to
the caller.

Remove the wasted work, and clean up some ALLOW_UNUSED_LOCAL() that are
no longer required, since DCHECKs in DataPipe() constructors were
replaced with CHECKs.

Bug: 892297,  890468 
Reviewed-on: https://chromium-review.googlesource.com/c/1266602
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#597623}
Change-Id: Ic83e3d4fe00808e1fd95acb283e62a2f9d78a3a4
Reviewed-on: https://chromium-review.googlesource.com/c/1277682
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#974}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/e35b1ad216fa7ef04b66177d49f692976ad8bf5d/mojo/public/cpp/system/data_pipe.h

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/e35b1ad216fa7ef04b66177d49f692976ad8bf5d

Commit: e35b1ad216fa7ef04b66177d49f692976ad8bf5d
Author: jam@chromium.org
Commiter: jam@chromium.org
Date: 2018-10-11 22:28:27 +0000 UTC

Fix DataPipe(capacity) constructor not to double-allocate.

The DataPipe(capacity) constructor was previously creating a DataPipe
on the stack, using the DataPipe(options) constructor, but then not
using it, and instead manually creating the pipe handles to return to
the caller.

Remove the wasted work, and clean up some ALLOW_UNUSED_LOCAL() that are
no longer required, since DCHECKs in DataPipe() constructors were
replaced with CHECKs.

Bug: 892297,  890468 
Reviewed-on: https://chromium-review.googlesource.com/c/1266602
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#597623}
Change-Id: Ic83e3d4fe00808e1fd95acb283e62a2f9d78a3a4
Reviewed-on: https://chromium-review.googlesource.com/c/1277682
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#974}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}