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

content/browser/webui/url_data_manager_backend.cc

Index: content/browser/webui/url_data_manager_backend.cc
diff --git a/content/browser/webui/url_data_manager_backend.cc b/content/browser/webui/url_data_manager_backend.cc
index f600b409bbbb24c010dedae1412bfbd447282e33..09d5a24ff0af82146ce96ce79daf0dab47d1087b 100644
--- a/content/browser/webui/url_data_manager_backend.cc
+++ b/content/browser/webui/url_data_manager_backend.cc
@@ -399,20 +399,19 @@ void URLRequestChromeJob::DataAvailable(base::RefCountedMemory* bytes) {
   TRACE_EVENT_ASYNC_END0("browser", "DataManager:Request", this);
   DCHECK(!data_);
 
-  // A passed-in nullptr signals an error.
-  if (!bytes) {
-    ReadRawDataComplete(net::ERR_FAILED);
-    return;
-  }
-
   // All further requests will be satisfied from the passed-in data.
   data_ = bytes;
 
   if (pending_buf_) {
-    int result = PostReadTask(pending_buf_, pending_buf_size_);
+    int result =
+        bytes ? PostReadTask(pending_buf_, pending_buf_size_) : net::ERR_FAILED;
     pending_buf_ = nullptr;
     if (result != net::ERR_IO_PENDING)
       ReadRawDataComplete(result);
+  } else if (!bytes) {
+    // TODO(pkasting): This doesn't seem right, we want FAILED, not CANCELED.
+    // NotifyDone() could do that, but we can't access it.
+    NotifyCanceled();

[mmenke, 2016/08/27 12:56:52]

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]

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:32]

"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.
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]

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:07]

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]

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]

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:43]

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:41]

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

   }
 }