[MD Settings] Remove jank for webui.

content/browser/webui/url_data_manager_backend.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/webui/url_data_manager_backend.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/threading/worker_pool.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/histogram_internals_request_job.h"
 #include "content/browser/net/view_blob_internals_job_factory.h"
 #include "content/browser/net/view_http_cache_job_factory.h"
 #include "content/browser/resource_context_impl.h"
 #include "content/browser/webui/shared_resources_data_source.h"
 #include "content/browser/webui/url_data_source_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 62 matching lines @@
   std::string result;
   if (request->extra_request_headers().GetHeader(
           net::HttpRequestHeaders::kOrigin, &result))
     return result;
   net::HttpRequestHeaders headers;
   if (request->GetFullRequestHeaders(&headers))
     headers.GetHeader(net::HttpRequestHeaders::kOrigin, &result);
   return result;
 }
 
+// Copy data from source buffer into IO buffer destination.
+// TODO(groby): Very similar to URLRequestSimpleJob, unify at some point.
+void CopyData(const scoped_refptr<net::IOBuffer>& buf,
+              int buf_size,
+              const scoped_refptr<base::RefCountedMemory>& data,
+              int64_t data_offset) {
+  // TODO(pkasting): Remove ScopedTracker below once crbug.com/455423 is
+  // fixed.
+  tracked_objects::ScopedTracker tracking_profile(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "455423 URLRequestChromeJob::CompleteRead memcpy"));
+  memcpy(buf->data(), data->front() + data_offset, buf_size);
+}
+
 }  // namespace
 
 // URLRequestChromeJob is a net::URLRequestJob that manages running
 // chrome-internal resource requests asynchronously.
 // It hands off URL requests to ChromeURLDataManager, which asynchronously
 // calls back once the data is available.
 class URLRequestChromeJob : public net::URLRequestJob {
  public:
   // |is_incognito| set when job is generated from an incognito profile.
   URLRequestChromeJob(net::URLRequest* request,
@@ skipping 80 matching lines @@
   // (This pattern is shared by most net::URLRequestJob implementations.)
   void StartAsync();
 
   // Due to a race condition, DevTools relies on a legacy thread hop to the UI
   // thread before calling StartAsync.
   // TODO(caseq): Fix the race condition and remove this thread hop in
   // https://crbug.com/616641.
   static void DelayStartForDevTools(
       const base::WeakPtr<URLRequestChromeJob>& job);
 
-  // Do the actual copy from data_ (the data we're serving) into |buf|.
-  // Separate from ReadRawData so we can handle async I/O. Returns the number of
-  // bytes read.
-  int CompleteRead(net::IOBuffer* buf, int buf_size);
+  // Post a task to copy |data_| to |buf_| on a worker thread, to avoid browser
+  // jank. (|data_| might be mem-mapped, so a memcpy can trigger file ops).
+  int PostReadTask(net::IOBuffer* buf, int buf_size);
 
   // The actual data we're serving.  NULL until it's been fetched.
   scoped_refptr<base::RefCountedMemory> data_;
   // The current offset into the data that we're handing off to our
   // callers via the Read interfaces.
   int data_offset_;
 
   // For async reads, we keep around a pointer to the buffer that
   // we're reading into.
   scoped_refptr<net::IOBuffer> pending_buf_;
@@ skipping 136 matching lines @@
   }
 }
 
 void URLRequestChromeJob::MimeTypeAvailable(const std::string& mime_type) {
   set_mime_type(mime_type);
   NotifyHeadersComplete();
 }
 
 void URLRequestChromeJob::DataAvailable(base::RefCountedMemory* bytes) {
   TRACE_EVENT_ASYNC_END0("browser", "DataManager:Request", this);
-  if (bytes) {
-    data_ = bytes;
-    if (pending_buf_.get()) {
-      CHECK(pending_buf_->data());
-      int result = CompleteRead(pending_buf_.get(), pending_buf_size_);
-      pending_buf_ = NULL;
+  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_.get(), pending_buf_size_);
+    if (result != net::ERR_IO_PENDING)
       ReadRawDataComplete(result);
-    }
-  } else {
-    // The request failed.
-    ReadRawDataComplete(net::ERR_FAILED);
+    pending_buf_ = nullptr;

[dproy, 2016/07/27 18:15:23]

Drive-by comment since we had to revert this CL because of crbug.com/631812: 
I was interested in understanding better why this line was crashing chrome and
it seems ReadRawDataComplete destroys the URLRequestJob, including its member
variable pending_buf_. When we assign nullptr to pending_buf_ in the next line,
it tries to release whatever pending_buf_ was pointing to, which now happens to
be garbage memory, and so we get a segfault.

Perhaps moving the nullptr assignment up two lines will fix the issue? This is
how it worked before; the null assignment was done before ReadRawDataComplete. 

I don't know to full context of this CL so cannot say definitively if this is a
proper fix, but I checked that this prevents the crash. Hope this helps. 

[groby-ooo-7-16, 2016/08/22 17:20:24]

That does seem to be the issue. Finally investigating, thanks for the heads-up!

[mmenke, 2016/08/22 17:25:52]

Hrm...Surely some of our browser tests run this code.  Wonder why they didn't
catch this.

   }
 }
 
 base::WeakPtr<URLRequestChromeJob> URLRequestChromeJob::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
 int URLRequestChromeJob::ReadRawData(net::IOBuffer* buf, int buf_size) {
+  DCHECK(!pending_buf_.get());
+
+  // If data isn't available yet, mark this as asynchronous.
   if (!data_.get()) {
-    DCHECK(!pending_buf_.get());
-    CHECK(buf->data());
     pending_buf_ = buf;
     pending_buf_size_ = buf_size;
     return net::ERR_IO_PENDING;
   }
 
-  // Otherwise, the data is available.
-  return CompleteRead(buf, buf_size);
+  return PostReadTask(buf, buf_size);
 }
 
-int URLRequestChromeJob::CompleteRead(net::IOBuffer* buf, int buf_size) {
+int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) {
+  DCHECK(buf);
+  DCHECK(data_);
+  CHECK(buf->data());
+
   int remaining = data_->size() - data_offset_;
   if (buf_size > remaining)
     buf_size = remaining;
-  if (buf_size > 0) {
-    // TODO(pkasting): Remove ScopedTracker below once crbug.com/455423 is
-    // fixed.
-    tracked_objects::ScopedTracker tracking_profile(
-        FROM_HERE_WITH_EXPLICIT_FUNCTION(
-            "455423 URLRequestChromeJob::CompleteRead memcpy"));
-    memcpy(buf->data(), data_->front() + data_offset_, buf_size);
-    data_offset_ += buf_size;
-  }
-  return buf_size;
+
+  if (buf_size == 0)
+    return 0;
+
+  base::WorkerPool::GetTaskRunner(false)->PostTaskAndReply(

[mmenke, 2016/07/20 18:19:44]

I think this should be true?  Potential disk accesses are considered slow, I
assume.

[groby-ooo-7-16, 2016/07/26 02:37:45]

I'm basing this on URLRequestSimpleJob - that also uses false. I've asked
chromium-dev, but no answer so far. I'll leave it for now.

+      FROM_HERE, base::Bind(&CopyData, base::RetainedRef(buf), buf_size, data_,
+                            data_offset_),
+      base::Bind(&URLRequestChromeJob::ReadRawDataComplete, AsWeakPtr(),
+                 buf_size));
+  data_offset_ += buf_size;
+
+  return net::ERR_IO_PENDING;
 }
 
 void URLRequestChromeJob::DelayStartForDevTools(
     const base::WeakPtr<URLRequestChromeJob>& job) {
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&URLRequestChromeJob::StartAsync, job));
 }
 
@@ skipping 372 matching lines @@
 
 }  // namespace
 
 net::URLRequestJobFactory::ProtocolHandler*
 CreateDevToolsProtocolHandler(content::ResourceContext* resource_context,
                               bool is_incognito) {
   return new DevToolsJobFactory(resource_context, is_incognito);
 }
 
 }  // namespace content