RDH: Ensure there's at most one LoadInfoList in flight.

ResourceDispatcherHost pushes pending load state of all frames to the UI
thread once every 250 milliseconds when there's a pending load.  This
happens even when the UI thread is blocked.  Apparently it's causing an
OOM when the UI thread is blocked.  Whatever is blocking the UI thread
is another issue entirely, but we shouldn't use unbounded memory in the
browser process.  This CL prevents load state updates from being sent
until the previous set was ACKed by the UI thread.

Bug: 824869
TBR: jam@chromium.org
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I3b68522c5281d477d3a621ac02e5df1f75153d99
Reviewed-on: https://chromium-review.googlesource.com/978383
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545616}

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -337,7 +337,8 @@ ResourceDispatcherHostImpl::ResourceDispatcherHostImpl(
       allow_cross_origin_auth_prompt_(false),
       create_download_handler_intercept_(download_handler_intercept),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      io_thread_task_runner_(io_thread_runner) {
+      io_thread_task_runner_(io_thread_runner),
+      weak_ptr_factory_(this) {
   DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
   DCHECK(!g_resource_dispatcher_host);
   g_resource_dispatcher_host = this;
@@ -350,7 +351,7 @@ ResourceDispatcherHostImpl::ResourceDispatcherHostImpl(
       FROM_HERE, base::BindOnce(&ResourceDispatcherHostImpl::OnInit,
                                 base::Unretained(this)));
 
-  update_load_states_timer_ = std::make_unique<base::RepeatingTimer>();
+  update_load_info_timer_ = std::make_unique<base::OneShotTimer>();
 
   // Monitor per-tab outstanding requests only if OOPIF is not enabled, because
   // the routing id doesn't represent tabs in OOPIF modes.
@@ -601,12 +602,7 @@ bool ResourceDispatcherHostImpl::HandleExternalProtocol(ResourceLoader* loader,
 
 void ResourceDispatcherHostImpl::DidStartRequest(ResourceLoader* loader) {
   // Make sure we have the load state monitors running.
-  if (!update_load_states_timer_->IsRunning() &&
-      scheduler_->DeprecatedHasLoadingClients()) {
-    update_load_states_timer_->Start(
-        FROM_HERE, TimeDelta::FromMilliseconds(kUpdateLoadStatesIntervalMsec),
-        this, &ResourceDispatcherHostImpl::UpdateLoadInfo);
-  }
+  MaybeStartUpdateLoadInfoTimer();
   if (record_outstanding_requests_stats_timer_ &&
       !record_outstanding_requests_stats_timer_->IsRunning()) {
     record_outstanding_requests_stats_timer_->Start(
@@ -721,7 +717,7 @@ void ResourceDispatcherHostImpl::OnShutdown() {
   // Make sure we shutdown the timers now, otherwise by the time our destructor
   // runs if the timer is still running the Task is deleted twice (once by
   // the MessageLoop and the second time by RepeatingTimer).
-  update_load_states_timer_.reset();
+  update_load_info_timer_.reset();
   record_outstanding_requests_stats_timer_.reset();
 
   // Clear blocked requests if any left.
@@ -2398,31 +2394,41 @@ ResourceDispatcherHostImpl::GetInterestingPerFrameLoadInfos() {
     }
   }
 
-  for (auto it : frame_infos)
+  for (auto it : frame_infos) {
     infos->push_back(std::move(it.second));
+  }
   return infos;
 }
 
 void ResourceDispatcherHostImpl::UpdateLoadInfo() {
   std::unique_ptr<LoadInfoList> infos(GetInterestingPerFrameLoadInfos());
 
-  // Stop the timer if there are no more pending requests. Future new requests
-  // will restart it as necessary.
-  // Also stop the timer if there are no loading clients, to avoid waking up
-  // unnecessarily when there is a long running (hanging get) request.
-  if (infos->empty() || !scheduler_->DeprecatedHasLoadingClients()) {
-    update_load_states_timer_->Stop();
-    return;
-  }
-
   // We need to be able to compare all requests to find the most important one
   // per tab. Since some requests may be navigation requests and we don't have
   // their render frame routing IDs yet (which is what we have for subresource
   // requests), we must go to the UI thread and compare the requests using their
   // WebContents.
-  main_thread_task_runner_->PostTask(
+  waiting_on_load_state_ack_ = true;
+  main_thread_task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::BindOnce(UpdateLoadStateOnUI, loader_delegate_, std::move(infos)));
+      base::BindOnce(UpdateLoadStateOnUI, loader_delegate_, std::move(infos)),
+      base::BindOnce(&ResourceDispatcherHostImpl::AckUpdateLoadInfo,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ResourceDispatcherHostImpl::AckUpdateLoadInfo() {
+  DCHECK(waiting_on_load_state_ack_);
+  waiting_on_load_state_ack_ = false;
+  MaybeStartUpdateLoadInfoTimer();
+}
+
+void ResourceDispatcherHostImpl::MaybeStartUpdateLoadInfoTimer() {
+  if (!waiting_on_load_state_ack_ && !update_load_info_timer_->IsRunning() &&
+      scheduler_->DeprecatedHasLoadingClients() && !pending_loaders_.empty()) {
+    update_load_info_timer_->Start(
+        FROM_HERE, TimeDelta::FromMilliseconds(kUpdateLoadStatesIntervalMsec),
+        this, &ResourceDispatcherHostImpl::UpdateLoadInfo);
+  }
 }
 
 void ResourceDispatcherHostImpl::RecordOutstandingRequestsStats() {

content/browser/loader/resource_dispatcher_host_impl.h

@@ -23,6 +23,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/single_thread_task_runner.h"
@@ -47,6 +48,7 @@
 
 namespace base {
 class FilePath;
+class OneShotTimer;
 class RepeatingTimer;
 }
 
@@ -534,6 +536,14 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
   // Checks all pending requests and updates the load info if necessary.
   void UpdateLoadInfo();
 
+  // Invoked on the IO thread once load state has been updated on the UI thread,
+  // starts timer call UpdateLoadInfo() again, if needed.
+  void AckUpdateLoadInfo();
+
+  // Starts the timer to call UpdateLoadInfo(), if timer isn't already running,
+  // |waiting_on_load_state_ack_| is false, and there are live ResourceLoaders.
+  void MaybeStartUpdateLoadInfoTimer();
+
   // Records statistics about outstanding requests since the last call, and
   // reset the stats.
   void RecordOutstandingRequestsStats();
@@ -718,9 +728,13 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
       RegisteredTempFiles;  // key is child process id
   RegisteredTempFiles registered_temp_files_;
 
-  // A timer that periodically calls UpdateLoadInfo while pending_loaders_ is
-  // not empty and at least one RenderViewHost is loading.
-  std::unique_ptr<base::RepeatingTimer> update_load_states_timer_;
+  // A timer that periodically calls UpdateLoadInfo while |pending_loaders_| is
+  // not empty, at least one RenderViewHost is loading, and not waiting on an
+  // ACK from the UI thread for the last sent LoadInfoList.
+  std::unique_ptr<base::OneShotTimer> update_load_info_timer_;
+  // True if a LoadInfoList has been sent to the UI thread, but has yet to be
+  // acknowledged.
+  bool waiting_on_load_state_ack_ = false;
 
   // A timer that periodically calls RecordOutstandingRequestsStats.
   std::unique_ptr<base::RepeatingTimer>
@@ -825,6 +839,8 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
   // Task runner for the IO thead.
   scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner_;
 
+  base::WeakPtrFactory<ResourceDispatcherHostImpl> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ResourceDispatcherHostImpl);
 };
 

content/browser/web_contents/web_contents_impl_browsertest.cc

@@ -1782,6 +1782,9 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateTargetURL) {
             target_url_waiter.WaitForUpdatedTargetURL());
 }
 
+// TODO(mmenke):  Beef up testing of LoadState a little. In particular, check
+// LoadState itself, not just the host name, check upload progress, check the
+// param, and make sure RDH pushes the data to the browser process.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateLoadState) {
   // Controlled responses for image requests made in the test. They will
   // alternate being the "most interesting" for the purposes of notifying the