Prevent imbalanced keepalive counts in ProcessManager

URLRequests can be canceled (e.g. on destruction) before
they're ever started, meaning that a NetworkDelegate
may be notified of request completion without ever being
notified of request start.

extensions::ProcessManager was making the incorrect assumption
that this couldn't happen (i.e. that a completion notification
must always follow a corresponding start notification), and was
indiscriminately decrementing keepalive count on all URLRequest
completion notifications.

This CL fixes the glitch.

BUG=535716
R=rdevlin.cronin@chromium.org

Review URL: https://codereview.chromium.org/1366393002

Cr-Commit-Position: refs/heads/master@{#351198}

chrome/browser/extensions/process_manager_browsertest.cc

@@ -157,4 +157,40 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, HttpHostMatchingExtensionId) {
   EXPECT_TRUE(pm->GetBackgroundHostForExtension(extension->id()));
 }
 
+// Verify correct keepalive count behavior on network request events.
+// Regression test for http://crbug.com/535716.
+IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, KeepaliveOnNetworkRequest) {
+  // Load an extension with a lazy background page.
+  scoped_refptr<const Extension> extension =
+      LoadExtension(test_data_dir_.AppendASCII("api_test")
+                        .AppendASCII("lazy_background_page")
+                        .AppendASCII("broadcast_event"));
+  ASSERT_TRUE(extension.get());
+
+  ProcessManager* pm = ProcessManager::Get(profile());
+  ProcessManager::FrameSet frames =
+      pm->GetRenderFrameHostsForExtension(extension->id());
+  ASSERT_EQ(1u, frames.size());
+
+  // Keepalive count at this point is unpredictable as there may be an
+  // outstanding event dispatch. We use the current keepalive count as a
+  // reliable baseline for future expectations.
+  int baseline_keepalive = pm->GetLazyKeepaliveCount(extension.get());
+
+  // Simulate some network events. This test assumes no other network requests
+  // are pending, i.e., that there are no conflicts with the fake request IDs
+  // we're using. This should be a safe assumption because LoadExtension should
+  // wait for loads to complete, and we don't run the message loop otherwise.
+  content::RenderFrameHost* frame_host = *frames.begin();
+  pm->OnNetworkRequestStarted(frame_host, 1);
+  EXPECT_EQ(baseline_keepalive + 1, pm->GetLazyKeepaliveCount(extension.get()));
+  pm->OnNetworkRequestDone(frame_host, 1);
+  EXPECT_EQ(baseline_keepalive, pm->GetLazyKeepaliveCount(extension.get()));
+
+  // Simulate only a request completion for this ID and ensure it doesn't result
+  // in keepalive decrement.
+  pm->OnNetworkRequestDone(frame_host, 2);
+  EXPECT_EQ(baseline_keepalive, pm->GetLazyKeepaliveCount(extension.get()));
+}
+
 }  // namespace extensions

extensions/browser/process_manager.cc

@@ -553,6 +553,8 @@ void ProcessManager::OnNetworkRequestStarted(
     uint64 request_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(
       GetExtensionID(render_frame_host));
+  auto result = pending_network_requests_.insert(request_id);
+  DCHECK(result.second) << "Duplicate network request IDs.";
   if (host && IsFrameInExtensionHost(host, render_frame_host)) {
     IncrementLazyKeepaliveCount(host->extension());
     host->OnNetworkRequestStarted(request_id);
@@ -566,7 +568,8 @@ void ProcessManager::OnNetworkRequestDone(
       GetExtensionID(render_frame_host));
   if (host && IsFrameInExtensionHost(host, render_frame_host)) {
     host->OnNetworkRequestDone(request_id);
-    DecrementLazyKeepaliveCount(host->extension());
+    if (pending_network_requests_.erase(request_id))
+      DecrementLazyKeepaliveCount(host->extension());
   }
 }
 

extensions/browser/process_manager.h

@@ -314,6 +314,13 @@ class ProcessManager : public KeyedService,
   // reset.
   uint64 last_background_close_sequence_id_;
 
+  // Tracks pending network requests by opaque ID. This is used to ensure proper
+  // keepalive counting in response to request status updates; e.g., if an
+  // extension URLRequest is constructed and then destroyed without ever
+  // starting, we can receive a completion notification without a corresponding
+  // start notification. In that case we want to avoid decrementing keepalive.
+  std::set<int> pending_network_requests_;
+
   // Must be last member, see doc on WeakPtrFactory.
   base::WeakPtrFactory<ProcessManager> weak_ptr_factory_;