Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously

when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=687240
Review-Url: https://codereview.chromium.org/2738973002
Cr-Commit-Position: refs/heads/master@{#458093}
Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8479a12a2f8aa

[mmenke, 2017-03-08 22:58:59]

Description was changed from

==========
stuff

BUG=
==========

to

==========
when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=
==========

[mmenke, 2017-03-08 22:59:05]

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

[commit-bot: I haz the power, 2017-03-08 23:00:02]

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-03-08 23:33:57]

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

[commit-bot: I haz the power, 2017-03-08 23:33:57]

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_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2017-03-09 21:35:15]

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

[commit-bot: I haz the power, 2017-03-09 21:35:51]

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

[mmenke, 2017-03-09 22:01:49]

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

[commit-bot: I haz the power, 2017-03-09 22:03:04]

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

[mmenke, 2017-03-09 22:08:16]

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

[commit-bot: I haz the power, 2017-03-09 22:08:40]

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-03-09 22:56:08]

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

[commit-bot: I haz the power, 2017-03-09 22:56:09]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-03-10 22:03:45]

mmenke@chromium.org changed reviewers:
+ rdsmith@chromium.org

[mmenke, 2017-03-10 22:03:45]

No rush to review this - figure I'll slowly continue work here in the background
as I focus on Mojo stuff, until everything's in a reasonable state.

The old code allocated a buff in the first OnWillRead call (Which may have been
an intermediary buffer or the actual mojo buffer), and subsequently always
allocated a buffer at the end of OnReadCompleted (Which was always a Mojo
buffer, and possibly smaller than kMinBufferSize).  The new code allocates
allocates a buffer in OnWillRead, and only forces the buffer to be at least
kMinBufferSize on the first OnWillRead call.

The new code isn't really any simpler, because we still need to enforce
kMinBufferSize on the first read, but if we move that logic to the
MimeSniffingResourceHandler, this code becomes a lot simpler (And MineSniffingRH
does, too, actually, so it's a win-win).

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:968:
ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnReadCompleted(0));
I assume that this was to skip past the initial non-mojo buffer read.  The new
code DCHECKs if there's another OnWillRead call after OnReadCompleted(0) (Since
buffer_ is non-NULL).  Could change that, but I think it's reasonable to DCHECK
when that happens.

[mmenke, 2017-03-10 22:12:22]

One other change of note:  Previously, BeginWrite / EndWrite errors would return
ERR_INSUFFICIENT_MEMORY if they happened in OnWillStart, but ERR_ABORTED or
ERR_FAILED in OnReadCompleted.  I've switched to always using
ERR_INSUFFICIENT_MEMORY - don't feel that strongly about that vs ERR_FAILED, but
we should be consistent, and ERR_ABORTED seems pretty clearly incorrect.

[mmenke, 2017-03-10 22:25:47]

Description was changed from

==========
when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=
==========

to

==========
when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=687240
==========

[mmenke, 2017-03-10 22:27:14]

[+yhirano]:  Just FYI.  Not that far from this CL to getting rid of the silly
minimum buffer size logic in MojoAsyncResourceHandler, I hope.

[Randy Smith (Not in Mondays), 2017-03-11 16:23:45]

On 2017/03/10 22:27:14, mmenke (getting caught up) wrote:
> [+yhirano]:  Just FYI.  Not that far from this CL to getting rid of the silly
> minimum buffer size logic in MojoAsyncResourceHandler, I hope.

Heh.  I'm working on a CL to get rid of the minimum buffer size logic from the
other direction (make MimeSniffingResourceHandler more modular). 
https://codereview.chromium.org/2743723003, though it's not yet ready for
review.

[mmenke, 2017-03-17 17:02:12]

On 2017/03/11 16:23:45, Randy Smith (Not in Mondays) wrote:
> On 2017/03/10 22:27:14, mmenke (getting caught up) wrote:
> > [+yhirano]:  Just FYI.  Not that far from this CL to getting rid of the
silly
> > minimum buffer size logic in MojoAsyncResourceHandler, I hope.
> 
> Heh.  I'm working on a CL to get rid of the minimum buffer size logic from the
> other direction (make MimeSniffingResourceHandler more modular). 
> https://codereview.chromium.org/2743723003, though it's not yet ready for
> review.

Gentle ping.

I'm in no rush here, as I'm not writing any code at the moment, just want to
make sure this doesn't get dropped.

[Randy Smith (Not in Mondays), 2017-03-17 17:12:14]

On 2017/03/17 17:02:12, mmenke wrote:
> On 2017/03/11 16:23:45, Randy Smith (Not in Mondays) wrote:
> > On 2017/03/10 22:27:14, mmenke (getting caught up) wrote:
> > > [+yhirano]:  Just FYI.  Not that far from this CL to getting rid of the
> silly
> > > minimum buffer size logic in MojoAsyncResourceHandler, I hope.
> > 
> > Heh.  I'm working on a CL to get rid of the minimum buffer size logic from
the
> > other direction (make MimeSniffingResourceHandler more modular). 
> > https://codereview.chromium.org/2743723003, though it's not yet ready for
> > review.
> 
> Gentle ping.
> 
> I'm in no rush here, as I'm not writing any code at the moment, just want to
> make sure this doesn't get dropped.

Fair; sorry.  It's after a couple of other reviews that I'm putting first
because of the "no rush", and I'm trying to keep the servification stuff moving
forward for obvious reasons.  I'll try to get to it within the next 2-3 business
days.

[mmenke, 2017-03-17 17:33:09]

On 2017/03/17 17:12:14, Randy Smith (Not in Mondays) wrote:
> On 2017/03/17 17:02:12, mmenke wrote:
> > On 2017/03/11 16:23:45, Randy Smith (Not in Mondays) wrote:
> > > On 2017/03/10 22:27:14, mmenke (getting caught up) wrote:
> > > > [+yhirano]:  Just FYI.  Not that far from this CL to getting rid of the
> > silly
> > > > minimum buffer size logic in MojoAsyncResourceHandler, I hope.
> > > 
> > > Heh.  I'm working on a CL to get rid of the minimum buffer size logic from
> the
> > > other direction (make MimeSniffingResourceHandler more modular). 
> > > https://codereview.chromium.org/2743723003, though it's not yet ready for
> > > review.
> > 
> > Gentle ping.
> > 
> > I'm in no rush here, as I'm not writing any code at the moment, just want to
> > make sure this doesn't get dropped.
> 
> Fair; sorry.  It's after a couple of other reviews that I'm putting first
> because of the "no rush", and I'm trying to keep the servification stuff
moving
> forward for obvious reasons.  I'll try to get to it within the next 2-3
business
> days.

SGTM.  And yea, this is a good review to punt.

[Randy Smith (Not in Mondays), 2017-03-17 20:06:07]

I'm confused by the MOJO_RESULT_SHOULD_WAIT question below, but beyond that it's
looking fine.

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:308: // subsequent
OnWillRead calls.
Confirming: It seems what this means is that you're overloading that DCHECK() to
also mean "No reads after the EOF signal"; is that accurate?  If so, just to be
extra pedantic, a note next to the DCHECK that it also means that, to aid in
hopefully never to happen debugging?

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:813: if (result
== MOJO_RESULT_SHOULD_WAIT) {
Under what circumstances would this happen?  And if we've written the data on
the other side of the pipe, shouldn't it eventually show up?  The 'continue'
below seems wrong in that case--there's a character we're never checking.

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:835: // Same as
above, but after th efirst OnWriteable() call, BeginWrite() indicates
nit: "th efirst"

[mmenke, 2017-03-17 20:44:47]

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

[commit-bot: I haz the power, 2017-03-17 20:45:19]

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-03-17 20:47:45]

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

[commit-bot: I haz the power, 2017-03-17 20:47:46]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-03-17 21:36:51]

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

[commit-bot: I haz the power, 2017-03-17 21:37:32]

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

[mmenke, 2017-03-17 22:29:39]

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:308: // subsequent
OnWillRead calls.
On 2017/03/17 20:06:06, Randy Smith (Not in Mondays) wrote:
> Confirming: It seems what this means is that you're overloading that DCHECK()
to
> also mean "No reads after the EOF signal"; is that accurate?  If so, just to
be
> extra pedantic, a note next to the DCHECK that it also means that, to aid in
> hopefully never to happen debugging?

It's mostly because a buffer_ = nullptr here didn't seem worth the effort here,
and then when I ran into DCHECK on existing tests doing weird things without it,
I decided I liked the new DCHECK condition.

(Note that in practice, we'll always delete synchronously after this, since we
don't delay destruction on normal reads - only WriteToFileResourceHandler defers
teardown after sending 0-byte-reads downstream, I believe, and when it's in use,
we won't be calling OnReadCompleted)

Anyhow, I've added a comment.

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:813: if (result
== MOJO_RESULT_SHOULD_WAIT) {
On 2017/03/17 20:06:06, Randy Smith (Not in Mondays) wrote:
> Under what circumstances would this happen?  And if we've written the data on
> the other side of the pipe, shouldn't it eventually show up?  The 'continue'
> below seems wrong in that case--there's a character we're never checking.

You're absolutely right that this code is completely broken, and that it
shouldn't be skipping those final two checks.  You're not quite right about how
it's broken.

MOJO_RESULT_SHOULD_WAIT is not the same as ERR_IO_PENDING, I believe.  There are
(at least) three ways for async read interfaces to work:

1)  "If I don't have read data now, I'll populate this buffer later and call you
back when you have.  You have to keep the buffer alive in the meantime."  This
is how ERR_IO_PENDING works.

2)  "If I don't have read data now, I'll tell you when I do, and then you can
try and read again, with the same buffer or a shiny new one."  This is how
stream_socket_posix works (can't remember which way the windows one works -
Windows has socket APIs that work both ways).  This lets you avoid having to
keep a big ol' smelly buffer around for however long there's no data available.

3)  It's the pipe's buffer, so it can keep the buffer around or be smarter about
it.

The way this Mojo interface works is 2), so the way this should work is that it
tells us to call it again later.  We then spin the message loop (Hopefully
letting the other end of the socket catch up), and then we try reading again... 
We should not be calling OnWillRead again, or incrementing out position in the
test input, however.

Thanks so much for catching this!

https://codereview.chromium.org/2738973002/diff/40001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:835: // Same as
above, but after th efirst OnWriteable() call, BeginWrite() indicates
On 2017/03/17 20:06:06, Randy Smith (Not in Mondays) wrote:
> nit: "th efirst"

Done.

[commit-bot: I haz the power, 2017-03-17 23:20:18]

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

[commit-bot: I haz the power, 2017-03-17 23:20:19]

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

[Randy Smith (Not in Mondays), 2017-03-19 02:35:59]

Got it; thanks for the explanation.  That'll be good to know :-}.

LGTM.

[mmenke, 2017-03-20 15:36:06]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-20 15:36:29]

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-03-20 16:48:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490024166930210,
"parent_rev": "00473ceffd7e8195bd62645188bd65f1d6748e96", "commit_rev":
"2ba831790eccdf8fa9178eef72b8479a12a2f8aa"}

[commit-bot: I haz the power, 2017-03-20 16:48:59]

Description was changed from

==========
when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=687240
==========

to

==========
when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG=687240

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

[commit-bot: I haz the power, 2017-03-20 16:49:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8...

[yhirano, 2017-03-21 03:29:53]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2760223002/ by yhirano@chromium.org.

The reason for reverting is: This CL leads to a layout test crash.
virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html.