Fix DCHECK in debug build when navigating to an unknown chrome://theme/ URL.

Fix DCHECK in debug build when navigating to an unknown chrome://theme/ URL.

I believe this was introduced in https://codereview.chromium.org/1467603002 .
Previously, URLRequestChromeJob::DataAvailable() called NotifyDone() directly in
the case of a failed request, but it was changed to call ReadRawDataComplete(),
which is only appropriate when you've been reading data, i.e. the request is in
the IO_PENDING state.  (The function DCHECKs that that's the case, and that
DCHECK is what's failing.)

BUG=none
TEST=none

Committed: https://crrev.com/5b92337e2b6c788f54423686b6a95951339cf627
Cr-Commit-Position: refs/heads/master@{#416444}

[Peter Kasting, 2016-08-27 07:48:12]

pkasting@chromium.org changed reviewers:
+ dbeam@chromium.org, mmenke@chromium.org

[Peter Kasting, 2016-08-27 07:48:13]

dbeam: content/browser/webui/, chrome/browser/ui/webui/ OWNER
mmenke: net/ OWNER, knowledgeable input for question below

Both of you just looked at https://codereview.chromium.org/2268653002 that
touched this same code, so I'm hoping you have it paged in to some degree.

My TODO in the code makes it pretty clear that while this works, it doesn't seem
quite correct, semantically.  But I didn't find a way to properly fail, rather
than cancel, the request.  Suggestions welcome.

[Peter Kasting, 2016-08-27 07:48:19]

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

[commit-bot: I haz the power, 2016-08-27 07:48:43]

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-08-27 08:30:27]

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

[commit-bot: I haz the power, 2016-08-27 08:30:28]

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

[Peter Kasting, 2016-08-27 08:33:31]

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

[commit-bot: I haz the power, 2016-08-27 08:33:58]

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-08-27 09:58:57]

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

[commit-bot: I haz the power, 2016-08-27 09:58:58]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-27 12:56:53]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
For errors, this should be "OnRawReadComplete(net::ERR_<error name>);", where
the error code comes from net/base/net_error_list.h

[mmenke, 2016-08-27 13:00:33]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 12:56:52, mmenke (busy) wrote:
> For errors, this should be "OnRawReadComplete(net::ERR_<error name>);", where
> the error code comes from net/base/net_error_list.h

If the problem is that this class can't handle being deleted in a DataAvailable
call, then the ReadRawDataComplete call above is also wrong, as there's no
guarantee that the owner of the URLRequest won't delete it in response to this
call.

So either need to allow this class to be deleted synchronously, or add a
PostTask for the ReadRawDataComplete call.

[Peter Kasting, 2016-08-27 19:18:33]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 13:00:33, mmenke (busy) wrote:
> On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > For errors, this should be "OnRawReadComplete(net::ERR_<error name>);",
where
> > the error code comes from net/base/net_error_list.h

"OnRawReadComplete" isn't a string found in codesearch.  Do you mean
RawReadComplete?  That's the call I'm removing, that call is the whole problem.

> If the problem is that this class can't handle being deleted in a
DataAvailable
> call, then the ReadRawDataComplete call above is also wrong, as there's no
> guarantee that the owner of the URLRequest won't delete it in response to this
> call.

No, that is not the problem I'm solving.  Did you read my CL description?  I
tried to cover it.  I'll try again.

The issue is that when |pending_buf_| is null, we haven't done anything
asynchronously yet (ReadRawData() has not been called), so we haven't put the
request into IO_PENDING state.  So calling RawReadComplete() is wrong, because
we're handling a synchronous request.  So the DCHECK in that function fails.

[mmenke, 2016-08-27 20:21:25]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 19:18:32, Peter Kasting wrote:
> On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > For errors, this should be "OnRawReadComplete(net::ERR_<error name>);",
> where
> > > the error code comes from net/base/net_error_list.h
> 
> "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> RawReadComplete?  That's the call I'm removing, that call is the whole
problem.
> 
> > If the problem is that this class can't handle being deleted in a
> DataAvailable
> > call, then the ReadRawDataComplete call above is also wrong, as there's no
> > guarantee that the owner of the URLRequest won't delete it in response to
this
> > call.
> 
> No, that is not the problem I'm solving.  Did you read my CL description?  I
> tried to cover it.  I'll try again.
> 
> The issue is that when |pending_buf_| is null, we haven't done anything
> asynchronously yet (ReadRawData() has not been called), so we haven't put the
> request into IO_PENDING state.  So calling RawReadComplete() is wrong, because
> we're handling a synchronous request.  So the DCHECK in that function fails.


Ah, I saw the addition of the "On return, |this| may be deleted." comment, and
assumed that indicated the purpose of this CL, as opposed to being an unrelated
modification.

So this change seems problematic - if we have a posted task to notify the
request that we've seen the response headers, we'd need to cancel that, to make
sure it doesn't get run after cancellation.  I think the correct fix is to make
whatever triggers this callback is only invoked in response to a ReadRawData
call (The StartDataRequest / URLDataManagerBackend::CallStartRequest call?)

[mmenke, 2016-08-27 20:44:08]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 20:21:25, mmenke wrote:
> On 2016/08/27 19:18:32, Peter Kasting wrote:
> > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > For errors, this should be "OnRawReadComplete(net::ERR_<error name>);",
> > where
> > > > the error code comes from net/base/net_error_list.h
> > 
> > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > RawReadComplete?  That's the call I'm removing, that call is the whole
> problem.
> > 
> > > If the problem is that this class can't handle being deleted in a
> > DataAvailable
> > > call, then the ReadRawDataComplete call above is also wrong, as there's no
> > > guarantee that the owner of the URLRequest won't delete it in response to
> this
> > > call.
> > 
> > No, that is not the problem I'm solving.  Did you read my CL description?  I
> > tried to cover it.  I'll try again.
> > 
> > The issue is that when |pending_buf_| is null, we haven't done anything
> > asynchronously yet (ReadRawData() has not been called), so we haven't put
the
> > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
because
> > we're handling a synchronous request.  So the DCHECK in that function fails.
> 
> 
> Ah, I saw the addition of the "On return, |this| may be deleted." comment, and
> assumed that indicated the purpose of this CL, as opposed to being an
unrelated
> modification.
> 
> So this change seems problematic - if we have a posted task to notify the
> request that we've seen the response headers, we'd need to cancel that, to
make
> sure it doesn't get run after cancellation.  I think the correct fix is to
make
> whatever triggers this callback is only invoked in response to a ReadRawData
> call (The StartDataRequest / URLDataManagerBackend::CallStartRequest call?)

Alternatively, we could just remember the failure, and return the error
synchronously in response to the ReadRawData call, but it seems more complicated
than necessary, and the extra delay shouldn't be a huge deal.

[Peter Kasting, 2016-08-27 22:03:51]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 20:21:25, mmenke wrote:
> On 2016/08/27 19:18:32, Peter Kasting wrote:
> > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > For errors, this should be "OnRawReadComplete(net::ERR_<error name>);",
> > where
> > > > the error code comes from net/base/net_error_list.h
> > 
> > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > RawReadComplete?  That's the call I'm removing, that call is the whole
> problem.
> > 
> > > If the problem is that this class can't handle being deleted in a
> > DataAvailable
> > > call, then the ReadRawDataComplete call above is also wrong, as there's no
> > > guarantee that the owner of the URLRequest won't delete it in response to
> this
> > > call.
> > 
> > No, that is not the problem I'm solving.  Did you read my CL description?  I
> > tried to cover it.  I'll try again.
> > 
> > The issue is that when |pending_buf_| is null, we haven't done anything
> > asynchronously yet (ReadRawData() has not been called), so we haven't put
the
> > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
because
> > we're handling a synchronous request.  So the DCHECK in that function fails.
> 
> 
> Ah, I saw the addition of the "On return, |this| may be deleted." comment, and
> assumed that indicated the purpose of this CL, as opposed to being an
unrelated
> modification.
> 
> So this change seems problematic - if we have a posted task to notify the
> request that we've seen the response headers, we'd need to cancel that, to
make
> sure it doesn't get run after cancellation.

There are no response headers, AFAIK.  This isn't an HTTP request.  This is in
URLRequestChromeJob, so we're handling chrome:// URLs.  The response can be
async, but it all comes in at once (see how simple DataAvailable() and
ReadRawData() are regarding data buffering).

There's only two ways control flow happens:

* DataAvailable() is called before ReadRawData(); in this case we're not in
IO_PENDING yet, |pending_buf_| is null, and so the function just copies the data
into |data_|.  Then ReadRawData() is called, sees that |data_| is non-null, and
calls PostReadTask() immediately to finish things off.

* ReadRawData() is called before DataAvailable(); in this case it sets up
|pending_buf_|, puts the request into IO_PENDING state, and when DataAvailable()
is called, it calls PostReadTask() itself and then ReadRawDataComplete().

When an error occurs, the code currently handles it as if we're always in the
second case -- we skip the PostReadTask() call but call ReadRawDataComplete(). 
I'm trying to handle the first case.  One way, as you suggested, is to remember
the failure and return it later in ReadRawData(), but that's more complicated
than just marking the request as failed immediately so we don't even bother
calling ReadRawData().

Even if there were response headers, I don't think we can have posted any tasks
to notify anybody of anything in this case, since, as I noted, we've not even
been asked to read anything or put the request into the pending state yet.

I don't know that making the callback only invoked in response to ReadRawData()
is a good idea.  The obvious way to do that would be to have ReadRawData() post
a task that tells the backend to start handling the request, and give it the
callback.  But that's adding multiple round trips through the message loop
compared to today, and as noted above I don't think we actually buy anything
doing that.

[mmenke, 2016-08-27 22:28:05]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 22:03:51, Peter Kasting wrote:
> On 2016/08/27 20:21:25, mmenke wrote:
> > On 2016/08/27 19:18:32, Peter Kasting wrote:
> > > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > > For errors, this should be "OnRawReadComplete(net::ERR_<error
name>);",
> > > where
> > > > > the error code comes from net/base/net_error_list.h
> > > 
> > > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > > RawReadComplete?  That's the call I'm removing, that call is the whole
> > problem.
> > > 
> > > > If the problem is that this class can't handle being deleted in a
> > > DataAvailable
> > > > call, then the ReadRawDataComplete call above is also wrong, as there's
no
> > > > guarantee that the owner of the URLRequest won't delete it in response
to
> > this
> > > > call.
> > > 
> > > No, that is not the problem I'm solving.  Did you read my CL description? 
I
> > > tried to cover it.  I'll try again.
> > > 
> > > The issue is that when |pending_buf_| is null, we haven't done anything
> > > asynchronously yet (ReadRawData() has not been called), so we haven't put
> the
> > > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
> because
> > > we're handling a synchronous request.  So the DCHECK in that function
fails.
> > 
> > 
> > Ah, I saw the addition of the "On return, |this| may be deleted." comment,
and
> > assumed that indicated the purpose of this CL, as opposed to being an
> unrelated
> > modification.
> > 
> > So this change seems problematic - if we have a posted task to notify the
> > request that we've seen the response headers, we'd need to cancel that, to
> make
> > sure it doesn't get run after cancellation.
> 
> There are no response headers, AFAIK.  This isn't an HTTP request.  This is in
> URLRequestChromeJob, so we're handling chrome:// URLs.  The response can be
> async, but it all comes in at once (see how simple DataAvailable() and
> ReadRawData() are regarding data buffering).
> 
> There's only two ways control flow happens:
> 
> * DataAvailable() is called before ReadRawData(); in this case we're not in
> IO_PENDING yet, |pending_buf_| is null, and so the function just copies the
data
> into |data_|.  Then ReadRawData() is called, sees that |data_| is non-null,
and
> calls PostReadTask() immediately to finish things off.
> 
> * ReadRawData() is called before DataAvailable(); in this case it sets up
> |pending_buf_|, puts the request into IO_PENDING state, and when
DataAvailable()
> is called, it calls PostReadTask() itself and then ReadRawDataComplete().
> 
> When an error occurs, the code currently handles it as if we're always in the
> second case -- we skip the PostReadTask() call but call ReadRawDataComplete().

> I'm trying to handle the first case.  One way, as you suggested, is to
remember
> the failure and return it later in ReadRawData(), but that's more complicated
> than just marking the request as failed immediately so we don't even bother
> calling ReadRawData().
> 
> Even if there were response headers, I don't think we can have posted any
tasks
> to notify anybody of anything in this case, since, as I noted, we've not even
> been asked to read anything or put the request into the pending state yet.
> 
> I don't know that making the callback only invoked in response to
ReadRawData()
> is a good idea.  The obvious way to do that would be to have ReadRawData()
post
> a task that tells the backend to start handling the request, and give it the
> callback.  But that's adding multiple round trips through the message loop
> compared to today, and as noted above I don't think we actually buy anything
> doing that.

Right, but URLRequest calls Start(), which must respond with
NotifyHeadersComplete() before the consumer will start reading the body.  Then
the consumer calls into URLRequest::Read, which calls
URLRequestJob::ReadRawData, asking for the response body.  Then the
URLRequestJob must either return the data (Or an error) synchronously, or call
ReadRawDataComplete.

I think it makes the most sense to not ask for data until ReadRawData is called
(i.e., don't do whatever triggers the DataAvailable call).  That way, only one
thing is happening at a time, and the code is simpler and more robust.  The
current code asks for data before ReadRawData is complete, which doesn't seem
like a good idea.

Note that we wouldn't be adding any round trips, we'd just be delaying when we
start them, from when Start() is called (Or StartAsync?) to when ReadRawData is
called.  It's generally called very soon after NotifyHeadersComplete() is
called, so I don't think perf is a big issue here.

[mmenke, 2016-08-27 22:42:44]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 22:28:05, mmenke wrote:
> On 2016/08/27 22:03:51, Peter Kasting wrote:
> > On 2016/08/27 20:21:25, mmenke wrote:
> > > On 2016/08/27 19:18:32, Peter Kasting wrote:
> > > > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > > > For errors, this should be "OnRawReadComplete(net::ERR_<error
> name>);",
> > > > where
> > > > > > the error code comes from net/base/net_error_list.h
> > > > 
> > > > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > > > RawReadComplete?  That's the call I'm removing, that call is the whole
> > > problem.
> > > > 
> > > > > If the problem is that this class can't handle being deleted in a
> > > > DataAvailable
> > > > > call, then the ReadRawDataComplete call above is also wrong, as
there's
> no
> > > > > guarantee that the owner of the URLRequest won't delete it in response
> to
> > > this
> > > > > call.
> > > > 
> > > > No, that is not the problem I'm solving.  Did you read my CL
description? 
> I
> > > > tried to cover it.  I'll try again.
> > > > 
> > > > The issue is that when |pending_buf_| is null, we haven't done anything
> > > > asynchronously yet (ReadRawData() has not been called), so we haven't
put
> > the
> > > > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
> > because
> > > > we're handling a synchronous request.  So the DCHECK in that function
> fails.
> > > 
> > > 
> > > Ah, I saw the addition of the "On return, |this| may be deleted." comment,
> and
> > > assumed that indicated the purpose of this CL, as opposed to being an
> > unrelated
> > > modification.
> > > 
> > > So this change seems problematic - if we have a posted task to notify the
> > > request that we've seen the response headers, we'd need to cancel that, to
> > make
> > > sure it doesn't get run after cancellation.
> > 
> > There are no response headers, AFAIK.  This isn't an HTTP request.  This is
in
> > URLRequestChromeJob, so we're handling chrome:// URLs.  The response can be
> > async, but it all comes in at once (see how simple DataAvailable() and
> > ReadRawData() are regarding data buffering).
> > 
> > There's only two ways control flow happens:
> > 
> > * DataAvailable() is called before ReadRawData(); in this case we're not in
> > IO_PENDING yet, |pending_buf_| is null, and so the function just copies the
> data
> > into |data_|.  Then ReadRawData() is called, sees that |data_| is non-null,
> and
> > calls PostReadTask() immediately to finish things off.
> > 
> > * ReadRawData() is called before DataAvailable(); in this case it sets up
> > |pending_buf_|, puts the request into IO_PENDING state, and when
> DataAvailable()
> > is called, it calls PostReadTask() itself and then ReadRawDataComplete().
> > 
> > When an error occurs, the code currently handles it as if we're always in
the
> > second case -- we skip the PostReadTask() call but call
ReadRawDataComplete().
> 
> > I'm trying to handle the first case.  One way, as you suggested, is to
> remember
> > the failure and return it later in ReadRawData(), but that's more
complicated
> > than just marking the request as failed immediately so we don't even bother
> > calling ReadRawData().
> > 
> > Even if there were response headers, I don't think we can have posted any
> tasks
> > to notify anybody of anything in this case, since, as I noted, we've not
even
> > been asked to read anything or put the request into the pending state yet.
> > 
> > I don't know that making the callback only invoked in response to
> ReadRawData()
> > is a good idea.  The obvious way to do that would be to have ReadRawData()
> post
> > a task that tells the backend to start handling the request, and give it the
> > callback.  But that's adding multiple round trips through the message loop
> > compared to today, and as noted above I don't think we actually buy anything
> > doing that.
> 
> Right, but URLRequest calls Start(), which must respond with
> NotifyHeadersComplete() before the consumer will start reading the body.  Then
> the consumer calls into URLRequest::Read, which calls
> URLRequestJob::ReadRawData, asking for the response body.  Then the
> URLRequestJob must either return the data (Or an error) synchronously, or call
> ReadRawDataComplete.
> 
> I think it makes the most sense to not ask for data until ReadRawData is
called
> (i.e., don't do whatever triggers the DataAvailable call).  That way, only one
> thing is happening at a time, and the code is simpler and more robust.  The
> current code asks for data before ReadRawData is complete, which doesn't seem
> like a good idea.
> 
> Note that we wouldn't be adding any round trips, we'd just be delaying when we
> start them, from when Start() is called (Or StartAsync?) to when ReadRawData
is
> called.  It's generally called very soon after NotifyHeadersComplete() is
> called, so I don't think perf is a big issue here.

Also, note that there's no supported out-of-band way for a URLRequestJob to mark
itself as failed.  The NotifyCancelled approach doesn't work, for reasons
already mentioned.  The way things work is the URLRequest asks us for something,
and we respond by invoking one method in response, then it asks us again, we
respond, etc.  If it's not waiting on us for anything, we don't actually have a
method to invoke.

[Peter Kasting, 2016-08-28 00:27:42]

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
On 2016/08/27 22:28:05, mmenke wrote:
> On 2016/08/27 22:03:51, Peter Kasting wrote:
> > On 2016/08/27 20:21:25, mmenke wrote:
> > > On 2016/08/27 19:18:32, Peter Kasting wrote:
> > > > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > > > For errors, this should be "OnRawReadComplete(net::ERR_<error
> name>);",
> > > > where
> > > > > > the error code comes from net/base/net_error_list.h
> > > > 
> > > > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > > > RawReadComplete?  That's the call I'm removing, that call is the whole
> > > problem.
> > > > 
> > > > > If the problem is that this class can't handle being deleted in a
> > > > DataAvailable
> > > > > call, then the ReadRawDataComplete call above is also wrong, as
there's
> no
> > > > > guarantee that the owner of the URLRequest won't delete it in response
> to
> > > this
> > > > > call.
> > > > 
> > > > No, that is not the problem I'm solving.  Did you read my CL
description? 
> I
> > > > tried to cover it.  I'll try again.
> > > > 
> > > > The issue is that when |pending_buf_| is null, we haven't done anything
> > > > asynchronously yet (ReadRawData() has not been called), so we haven't
put
> > the
> > > > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
> > because
> > > > we're handling a synchronous request.  So the DCHECK in that function
> fails.
> > > 
> > > 
> > > Ah, I saw the addition of the "On return, |this| may be deleted." comment,
> and
> > > assumed that indicated the purpose of this CL, as opposed to being an
> > unrelated
> > > modification.
> > > 
> > > So this change seems problematic - if we have a posted task to notify the
> > > request that we've seen the response headers, we'd need to cancel that, to
> > make
> > > sure it doesn't get run after cancellation.
> > 
> > There are no response headers, AFAIK.  This isn't an HTTP request.  This is
in
> > URLRequestChromeJob, so we're handling chrome:// URLs.  The response can be
> > async, but it all comes in at once (see how simple DataAvailable() and
> > ReadRawData() are regarding data buffering).
> > 
> > There's only two ways control flow happens:
> > 
> > * DataAvailable() is called before ReadRawData(); in this case we're not in
> > IO_PENDING yet, |pending_buf_| is null, and so the function just copies the
> data
> > into |data_|.  Then ReadRawData() is called, sees that |data_| is non-null,
> and
> > calls PostReadTask() immediately to finish things off.
> > 
> > * ReadRawData() is called before DataAvailable(); in this case it sets up
> > |pending_buf_|, puts the request into IO_PENDING state, and when
> DataAvailable()
> > is called, it calls PostReadTask() itself and then ReadRawDataComplete().
> > 
> > When an error occurs, the code currently handles it as if we're always in
the
> > second case -- we skip the PostReadTask() call but call
ReadRawDataComplete().
> 
> > I'm trying to handle the first case.  One way, as you suggested, is to
> remember
> > the failure and return it later in ReadRawData(), but that's more
complicated
> > than just marking the request as failed immediately so we don't even bother
> > calling ReadRawData().
> > 
> > Even if there were response headers, I don't think we can have posted any
> tasks
> > to notify anybody of anything in this case, since, as I noted, we've not
even
> > been asked to read anything or put the request into the pending state yet.
> > 
> > I don't know that making the callback only invoked in response to
> ReadRawData()
> > is a good idea.  The obvious way to do that would be to have ReadRawData()
> post
> > a task that tells the backend to start handling the request, and give it the
> > callback.  But that's adding multiple round trips through the message loop
> > compared to today, and as noted above I don't think we actually buy anything
> > doing that.
> 
> Right, but URLRequest calls Start(), which must respond with
> NotifyHeadersComplete() before the consumer will start reading the body.  Then
> the consumer calls into URLRequest::Read, which calls
> URLRequestJob::ReadRawData, asking for the response body.  Then the
> URLRequestJob must either return the data (Or an error) synchronously, or call
> ReadRawDataComplete.
> 
> I think it makes the most sense to not ask for data until ReadRawData is
called
> (i.e., don't do whatever triggers the DataAvailable call).  That way, only one
> thing is happening at a time, and the code is simpler and more robust.  The
> current code asks for data before ReadRawData is complete, which doesn't seem
> like a good idea.

The critical call is URLDataSource::StartDataRequest().  That asks the backend
to be begin collecting data, and supplies it with a callback to call when that
data is available.  The callback can be invoked synchronously or asynchronously.

URLDataManagerBackend has a lot of complexity built in around calling this
method on the thread the callee expects, if necessary (some callees can handle
being on the IO thread or UI thread, some need one or the other).  See
URLDataManagerBackend::StartRequest().

I don't know what would be involved in engineering the change you request, as
I've literally never tried to work with anything in this stack before yesterday.
 I do not see an obvious way to make things "simpler" by doing what you request
-- at best, it seems like it would be no more complicated than today, by maybe
just moving a lot of the content of URLDataManagerBackend::StartRequest() to
ReadRawData().  That's if both those methods are always called on the same
thread, and nothing else breaks from doing that.  There are like 50 overrides of
StartDataRequest(), so it is wildly beyond me to know what kind of contracts
those all expect.

What I'm trying to convey here is that I've spent about 8 hours on this just to
get to the point I'm at now, and I'm rapidly feeling more, rather than less, out
of my depth.  Just trying to track control flow across all the different thread
hopping is really confusing.

> Note that we wouldn't be adding any round trips, we'd just be delaying when we
> start them, from when Start() is called (Or StartAsync?) to when ReadRawData
is
> called.  It's generally called very soon after NotifyHeadersComplete() is
> called, so I don't think perf is a big issue here.

Delaying when chrome URLs have their backing data fetched, even by small
amounts, seems dangerous to me.  This code is on the critical path for the NTP
being usable, which in turn is part of startup.  Even small amounts of increased
parallelism really matter.

I would be far more comfortable simply remembering the error passed to
DataAvailable() and returning it from ReadRawData().  That makes no change to
the time ordering of anything and is within my comprehension to engineer.

[mmenke, 2016-08-28 12:16:49]

On 2016/08/28 00:27:42, Peter Kasting wrote:
>
https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
> File content/browser/webui/url_data_manager_backend.cc (right):
> 
>
https://codereview.chromium.org/2279293004/diff/20001/content/browser/webui/u...
> content/browser/webui/url_data_manager_backend.cc:414: NotifyCanceled();
> On 2016/08/27 22:28:05, mmenke wrote:
> > On 2016/08/27 22:03:51, Peter Kasting wrote:
> > > On 2016/08/27 20:21:25, mmenke wrote:
> > > > On 2016/08/27 19:18:32, Peter Kasting wrote:
> > > > > On 2016/08/27 13:00:33, mmenke (busy) wrote:
> > > > > > On 2016/08/27 12:56:52, mmenke (busy) wrote:
> > > > > > > For errors, this should be "OnRawReadComplete(net::ERR_<error
> > name>);",
> > > > > where
> > > > > > > the error code comes from net/base/net_error_list.h
> > > > > 
> > > > > "OnRawReadComplete" isn't a string found in codesearch.  Do you mean
> > > > > RawReadComplete?  That's the call I'm removing, that call is the whole
> > > > problem.
> > > > > 
> > > > > > If the problem is that this class can't handle being deleted in a
> > > > > DataAvailable
> > > > > > call, then the ReadRawDataComplete call above is also wrong, as
> there's
> > no
> > > > > > guarantee that the owner of the URLRequest won't delete it in
response
> > to
> > > > this
> > > > > > call.
> > > > > 
> > > > > No, that is not the problem I'm solving.  Did you read my CL
> description? 
> > I
> > > > > tried to cover it.  I'll try again.
> > > > > 
> > > > > The issue is that when |pending_buf_| is null, we haven't done
anything
> > > > > asynchronously yet (ReadRawData() has not been called), so we haven't
> put
> > > the
> > > > > request into IO_PENDING state.  So calling RawReadComplete() is wrong,
> > > because
> > > > > we're handling a synchronous request.  So the DCHECK in that function
> > fails.
> > > > 
> > > > 
> > > > Ah, I saw the addition of the "On return, |this| may be deleted."
comment,
> > and
> > > > assumed that indicated the purpose of this CL, as opposed to being an
> > > unrelated
> > > > modification.
> > > > 
> > > > So this change seems problematic - if we have a posted task to notify
the
> > > > request that we've seen the response headers, we'd need to cancel that,
to
> > > make
> > > > sure it doesn't get run after cancellation.
> > > 
> > > There are no response headers, AFAIK.  This isn't an HTTP request.  This
is
> in
> > > URLRequestChromeJob, so we're handling chrome:// URLs.  The response can
be
> > > async, but it all comes in at once (see how simple DataAvailable() and
> > > ReadRawData() are regarding data buffering).
> > > 
> > > There's only two ways control flow happens:
> > > 
> > > * DataAvailable() is called before ReadRawData(); in this case we're not
in
> > > IO_PENDING yet, |pending_buf_| is null, and so the function just copies
the
> > data
> > > into |data_|.  Then ReadRawData() is called, sees that |data_| is
non-null,
> > and
> > > calls PostReadTask() immediately to finish things off.
> > > 
> > > * ReadRawData() is called before DataAvailable(); in this case it sets up
> > > |pending_buf_|, puts the request into IO_PENDING state, and when
> > DataAvailable()
> > > is called, it calls PostReadTask() itself and then ReadRawDataComplete().
> > > 
> > > When an error occurs, the code currently handles it as if we're always in
> the
> > > second case -- we skip the PostReadTask() call but call
> ReadRawDataComplete().
> > 
> > > I'm trying to handle the first case.  One way, as you suggested, is to
> > remember
> > > the failure and return it later in ReadRawData(), but that's more
> complicated
> > > than just marking the request as failed immediately so we don't even
bother
> > > calling ReadRawData().
> > > 
> > > Even if there were response headers, I don't think we can have posted any
> > tasks
> > > to notify anybody of anything in this case, since, as I noted, we've not
> even
> > > been asked to read anything or put the request into the pending state yet.
> > > 
> > > I don't know that making the callback only invoked in response to
> > ReadRawData()
> > > is a good idea.  The obvious way to do that would be to have ReadRawData()
> > post
> > > a task that tells the backend to start handling the request, and give it
the
> > > callback.  But that's adding multiple round trips through the message loop
> > > compared to today, and as noted above I don't think we actually buy
anything
> > > doing that.
> > 
> > Right, but URLRequest calls Start(), which must respond with
> > NotifyHeadersComplete() before the consumer will start reading the body. 
Then
> > the consumer calls into URLRequest::Read, which calls
> > URLRequestJob::ReadRawData, asking for the response body.  Then the
> > URLRequestJob must either return the data (Or an error) synchronously, or
call
> > ReadRawDataComplete.
> > 
> > I think it makes the most sense to not ask for data until ReadRawData is
> called
> > (i.e., don't do whatever triggers the DataAvailable call).  That way, only
one
> > thing is happening at a time, and the code is simpler and more robust.  The
> > current code asks for data before ReadRawData is complete, which doesn't
seem
> > like a good idea.
> 
> The critical call is URLDataSource::StartDataRequest().  That asks the backend
> to be begin collecting data, and supplies it with a callback to call when that
> data is available.  The callback can be invoked synchronously or
asynchronously.
> 
> URLDataManagerBackend has a lot of complexity built in around calling this
> method on the thread the callee expects, if necessary (some callees can handle
> being on the IO thread or UI thread, some need one or the other).  See
> URLDataManagerBackend::StartRequest().
> 
> I don't know what would be involved in engineering the change you request, as
> I've literally never tried to work with anything in this stack before
yesterday.
>  I do not see an obvious way to make things "simpler" by doing what you
request
> -- at best, it seems like it would be no more complicated than today, by maybe
> just moving a lot of the content of URLDataManagerBackend::StartRequest() to
> ReadRawData().  That's if both those methods are always called on the same
> thread, and nothing else breaks from doing that.  There are like 50 overrides
of
> StartDataRequest(), so it is wildly beyond me to know what kind of contracts
> those all expect.

Currently, we have 4 paths:

* Success before ReadRawData is called.
* Failure before ReadRawData is called.
* Success after ReadRawData is called.
* Failure after ReadRawData is called.

And it seems like we need additional code for each case, and a test for each
case, as the existence of this bug illustrates.  If we didn't start reading
until ReadRawData is invoked, we'd only have two paths, and the odds are this
bug would never have existed.  Fewer paths, fewer races, tends to lead to fewer
bugs, since there are fewer paths through the code, and each path is exercised
more.

The performance difference in the two paths should be minimal - we generally
don't do much in that period - I suspect there's just one other IO thread to IO
thread PostTask which is responsible for the race here.

Anyhow, I don't own this code, and am not going to insist on this path forward,
just giving my take on what's the most robust way to go.

> What I'm trying to convey here is that I've spent about 8 hours on this just
to
> get to the point I'm at now, and I'm rapidly feeling more, rather than less,
out
> of my depth.  Just trying to track control flow across all the different
thread
> hopping is really confusing.
> 
> > Note that we wouldn't be adding any round trips, we'd just be delaying when
we
> > start them, from when Start() is called (Or StartAsync?) to when ReadRawData
> is
> > called.  It's generally called very soon after NotifyHeadersComplete() is
> > called, so I don't think perf is a big issue here.
> 
> Delaying when chrome URLs have their backing data fetched, even by small
> amounts, seems dangerous to me.  This code is on the critical path for the NTP
> being usable, which in turn is part of startup.  Even small amounts of
increased
> parallelism really matter.
> 
> I would be far more comfortable simply remembering the error passed to
> DataAvailable() and returning it from ReadRawData().  That makes no change to
> the time ordering of anything and is within my comprehension to engineer.

[Dan Beam, 2016-08-29 17:54:37]

can we just change the DCHECK() in URLRequestJob::ReadRawDataComplete to

DCHECK(request_->status().is_io_pending() || result < 0);

I don't really see why this method cares about the status being in an IO_PENDING
state on error?

[mmenke, 2016-08-29 18:02:30]

On 2016/08/29 17:54:37, Dan Beam wrote:
> can we just change the DCHECK() in URLRequestJob::ReadRawDataComplete to
> 
> DCHECK(request_->status().is_io_pending() || result < 0);
> 
> I don't really see why this method cares about the status being in an
IO_PENDING
> state on error?

It needs to call into something to tell it about the error, the
URLRequest/URLRequestJob API is a pull-based API.  If there's nothing that's
even requested a read, a read can't logically have completed.  The code is not
written to support this.  We'd be calling into the consumer at an unexpected
point, and we'd be changing the URLRequest API itself.

[Peter Kasting, 2016-08-30 06:24:40]

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

[commit-bot: I haz the power, 2016-08-30 06:25:07]

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

[Peter Kasting, 2016-08-30 06:26:52]

I'm still test-compiling it, but here's the solution that stores an error code.

[commit-bot: I haz the power, 2016-08-30 08:25:05]

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

[commit-bot: I haz the power, 2016-08-30 08:25:06]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-30 14:42:38]

LGTM

https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc (right):

https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc:68: browser(),
GURL("chrome://theme/IDR_ASDFGHJKL"));
optional:  Maybe run Javascript to make sure the page's text contains
ERR_FAILED?  Admittedly, seems unlikely this will ever not be an error page, but
I'm paranoid.

Would be something like:

  std::string command = base::StringPrintf(
      "var textContent = document.body.innerText;"
      "var hasText = textContent.indexOf('ERR_FAILED') >= 0;"
      "domAutomationController.send(hasText);");
  bool result = false;
  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
      browser->tab_strip_model()->GetActiveWebContents(), command, &result));
  EXPECT_TRUE(result)

That does introduce a dependency on the error page displaying ERR_FAILED, of
course.

[Peter Kasting, 2016-08-30 19:46:07]

https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc (right):

https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc:68: browser(),
GURL("chrome://theme/IDR_ASDFGHJKL"));
On 2016/08/30 14:42:38, mmenke wrote:
> optional:  Maybe run Javascript to make sure the page's text contains
> ERR_FAILED?  Admittedly, seems unlikely this will ever not be an error page,
but
> I'm paranoid.
> 
> Would be something like:
> 
>   std::string command = base::StringPrintf(
>       "var textContent = document.body.innerText;"
>       "var hasText = textContent.indexOf('ERR_FAILED') >= 0;"
>       "domAutomationController.send(hasText);");
>   bool result = false;
>   EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
>       browser->tab_strip_model()->GetActiveWebContents(), command, &result));
>   EXPECT_TRUE(result)
> 
> That does introduce a dependency on the error page displaying ERR_FAILED, of
> course.

That's all true, but I think the more important question is whether we want this
to be testing that the specific result in this case is an error page of any
kind, and that the error code involved is ERR_FAILED.  I could see an argument
for doing this as a way of ensuring someone doesn't cause these requests to
erroneously succeed.  I could also see an argument that this is a bit of a
change detector test since we don't care how unknown resources are presented to
the user, just that they don't cause a crash.

I am on the fence.

[mmenke, 2016-08-30 19:55:02]

On 2016/08/30 19:46:07, Peter Kasting wrote:
>
https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2279293004/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc:68: browser(),
> GURL("chrome://theme/IDR_ASDFGHJKL"));
> On 2016/08/30 14:42:38, mmenke wrote:
> > optional:  Maybe run Javascript to make sure the page's text contains
> > ERR_FAILED?  Admittedly, seems unlikely this will ever not be an error page,
> but
> > I'm paranoid.
> > 
> > Would be something like:
> > 
> >   std::string command = base::StringPrintf(
> >       "var textContent = document.body.innerText;"
> >       "var hasText = textContent.indexOf('ERR_FAILED') >= 0;"
> >       "domAutomationController.send(hasText);");
> >   bool result = false;
> >   EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
> >       browser->tab_strip_model()->GetActiveWebContents(), command,
&result));
> >   EXPECT_TRUE(result)
> > 
> > That does introduce a dependency on the error page displaying ERR_FAILED, of
> > course.
> 
> That's all true, but I think the more important question is whether we want
this
> to be testing that the specific result in this case is an error page of any
> kind, and that the error code involved is ERR_FAILED.  I could see an argument
> for doing this as a way of ensuring someone doesn't cause these requests to
> erroneously succeed.  I could also see an argument that this is a bit of a
> change detector test since we don't care how unknown resources are presented
to
> the user, just that they don't cause a crash.
> 
> I am on the fence.

Yea, this is a completely valid concern.

I'd go with my suggestion (shockingly), but an error page UI change could well
break it (For instance, putting the error code below the fold, or removing the
ERR_ prefix).

Another option is to make a WebContentsObserver, and watch for
DidFinishNavigation for a NavigationHandle where IsErrorPage() is true (And
could even check the network error).  Or could leave well enough alone.

Anyhow, if you prefer none of these, that's fine with me.  I don't feel strongly
about this, and plenty of reasons to keep as-is.

[Peter Kasting, 2016-08-30 21:09:25]

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

[commit-bot: I haz the power, 2016-08-30 21:10:18]

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

[Peter Kasting, 2016-08-30 21:11:08]

On 2016/08/30 19:55:02, mmenke wrote:
> Another option is to make a WebContentsObserver, and watch for
> DidFinishNavigation for a NavigationHandle where IsErrorPage() is true (And
> could even check the network error).

I like the idea of checking IsErrorPage().  That's more robust than parsing the
text of the resulting page.  I uploaded a version which does that (and doesn't
check the specific network error; that still seems more like an implementation
detail).

[mmenke, 2016-08-30 21:14:01]

On 2016/08/30 21:11:08, Peter Kasting wrote:
> On 2016/08/30 19:55:02, mmenke wrote:
> > Another option is to make a WebContentsObserver, and watch for
> > DidFinishNavigation for a NavigationHandle where IsErrorPage() is true (And
> > could even check the network error).
> 
> I like the idea of checking IsErrorPage().  That's more robust than parsing
the
> text of the resulting page.  I uploaded a version which does that (and doesn't
> check the specific network error; that still seems more like an implementation
> detail).

LGTM

[Dan Beam, 2016-08-30 21:35:17]

lgtm

https://codereview.chromium.org/2279293004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc (right):

https://codereview.chromium.org/2279293004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc:55:
NON_ERROR_PAGE,
nit: NON_ERROR -> SUCCESS or something?

[Peter Kasting, 2016-08-30 21:55:55]

https://codereview.chromium.org/2279293004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc (right):

https://codereview.chromium.org/2279293004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_url_data_manager_browsertest.cc:55:
NON_ERROR_PAGE,
On 2016/08/30 21:35:17, Dan Beam wrote:
> nit: NON_ERROR -> SUCCESS or something?

Eschew unnecessary obfuscation?

SUCCESS is a perfectly good "navigation result".  Done.

[Peter Kasting, 2016-08-30 21:56:08]

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

[commit-bot: I haz the power, 2016-08-30 21:56:52]

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-08-30 23:17:54]

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

[commit-bot: I haz the power, 2016-08-30 23:17:55]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-03 02:57:30]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-09-03 02:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2279293004/#ps80001
(title: "Better name")

[commit-bot: I haz the power, 2016-09-03 02:57: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-09-03 04:12:16]

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

[commit-bot: I haz the power, 2016-09-03 04:12:17]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Kasting, 2016-09-03 04:17:46]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-09-03 04:17:57]

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-09-03 04:53:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-03 04:55:32]

Description was changed from

==========
Fix DCHECK in debug build when navigating to an unknown chrome://theme/ URL.

I believe this was introduced in https://codereview.chromium.org/1467603002 .
Previously, URLRequestChromeJob::DataAvailable() called NotifyDone() directly in
the case of a failed request, but it was changed to call ReadRawDataComplete(),
which is only appropriate when you've been reading data, i.e. the request is in
the IO_PENDING state.  (The function DCHECKs that that's the case, and that
DCHECK is what's failing.)

BUG=none
TEST=none
==========

to

==========
Fix DCHECK in debug build when navigating to an unknown chrome://theme/ URL.

I believe this was introduced in https://codereview.chromium.org/1467603002 .
Previously, URLRequestChromeJob::DataAvailable() called NotifyDone() directly in
the case of a failed request, but it was changed to call ReadRawDataComplete(),
which is only appropriate when you've been reading data, i.e. the request is in
the IO_PENDING state.  (The function DCHECKs that that's the case, and that
DCHECK is what's failing.)

BUG=none
TEST=none

Committed: https://crrev.com/5b92337e2b6c788f54423686b6a95951339cf627
Cr-Commit-Position: refs/heads/master@{#416444}
==========

[commit-bot: I haz the power, 2016-09-03 04:55:33]

Patchset 5 (id:??) landed as
https://crrev.com/5b92337e2b6c788f54423686b6a95951339cf627
Cr-Commit-Position: refs/heads/master@{#416444}