Clear testing callbacks in AppBackgrounPageNaclTest reentrantly.

Addresses a flaky ASAN failure where I believe callbacks from ImpulseCallbackCounter with an unretained reference were being held too long by ProcessManager.

This clears the callbacks immediately when the impulse count is reached, and does not call the quit closure repeatedly.

BUG=332440

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244247 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 21 insertions, 15 deletions

diff --git a/chrome/browser/extensions/app_background_page_apitest.cc b/chrome/browser/extensions/app_background_page_apitest.cc
index e89ae6c..9f6dea0 100644
--- a/chrome/browser/extensions/app_background_page_apitest.cc
+++ b/chrome/browser/extensions/app_background_page_apitest.cc
@@ -156,9 +156,11 @@ class AppBackgroundPageNaClTest : public AppBackgroundPageApiTest {
 // that will match a specified goal and can be waited on.
 class ImpulseCallbackCounter {
  public:
-  explicit ImpulseCallbackCounter(const std::string& extension_id)
+  explicit ImpulseCallbackCounter(extensions::ProcessManager* manager,
+                                  const std::string& extension_id)
       : observed_(0),
         goal_(0),
+        manager_(manager),
         extension_id_(extension_id) {
   }
 
@@ -183,6 +185,11 @@ class ImpulseCallbackCounter {
       const std::string& extension_id_from_manager) {
     if (extension_id_from_test == extension_id_from_manager) {
       if (++observed_ >= goal_) {
+        // Clear callback to free reference to message loop.
+        manager_->SetKeepaliveImpulseCallbackForTesting(
+            extensions::ProcessManager::ImpulseCallbackForTesting());
+        manager_->SetKeepaliveImpulseDecrementCallbackForTesting(
+            extensions::ProcessManager::ImpulseCallbackForTesting());
         quit_callback.Run();
       }
     }
@@ -190,6 +197,7 @@ class ImpulseCallbackCounter {
 
   int observed_;
   int goal_;
+  extensions::ProcessManager* manager_;
   const std::string extension_id_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 };
@@ -597,17 +605,13 @@ IN_PROC_BROWSER_TEST_F(AppBackgroundPageNaClTest, BackgroundKeepaliveActive) {
   LaunchTestingApp();
   extensions::ProcessManager* manager =
     extensions::ExtensionSystem::Get(browser()->profile())->process_manager();
-  ImpulseCallbackCounter active_impulse_counter(extension()->id());
+  ImpulseCallbackCounter active_impulse_counter(manager, extension()->id());
   EXPECT_TRUE(nacl_modules_loaded.WaitUntilSatisfied());
 
   // Target .5 seconds: .5 seconds / 50ms throttle * 2 embeds == 20 impulses.
   manager->SetKeepaliveImpulseCallbackForTesting(
       active_impulse_counter.SetGoalAndGetCallback(20));
   active_impulse_counter.Wait();
-
-  // Clear callback to free reference to message loop in ImpulseCallbackCounter.
-  manager->SetKeepaliveImpulseCallbackForTesting(
-      extensions::ProcessManager::ImpulseCallbackForTesting());
 }
 
 // Verify that nacl modules that go idle will not send keepalive impulses.
@@ -625,16 +629,12 @@ IN_PROC_BROWSER_TEST_F(AppBackgroundPageNaClTest,
   LaunchTestingApp();
   extensions::ProcessManager* manager =
     extensions::ExtensionSystem::Get(browser()->profile())->process_manager();
-  ImpulseCallbackCounter idle_impulse_counter(extension()->id());
+  ImpulseCallbackCounter idle_impulse_counter(manager, extension()->id());
   EXPECT_TRUE(nacl_modules_loaded.WaitUntilSatisfied());
 
   manager->SetKeepaliveImpulseDecrementCallbackForTesting(
       idle_impulse_counter.SetGoalAndGetCallback(1));
   nacl_modules_loaded.Reply("be idle");
   idle_impulse_counter.Wait();
-
-  // Clear callback to free reference to message loop in ImpulseCallbackCounter.
-  manager->SetKeepaliveImpulseDecrementCallbackForTesting(
-      extensions::ProcessManager::ImpulseCallbackForTesting());
 }
 
diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc
index a38f8bc..fed3369 100644
--- a/extensions/browser/process_manager.cc
+++ b/extensions/browser/process_manager.cc
@@ -443,8 +443,11 @@ void ProcessManager::KeepaliveImpulse(const Extension* extension) {
     }
   }
 
-  if (!keepalive_impulse_callback_for_testing_.is_null())
-    keepalive_impulse_callback_for_testing_.Run(extension->id());
+  if (!keepalive_impulse_callback_for_testing_.is_null()) {
+    ImpulseCallbackForTesting callback_may_clear_callbacks_reentrantly =
+      keepalive_impulse_callback_for_testing_;
+    callback_may_clear_callbacks_reentrantly.Run(extension->id());
+  }
 }
 
 // DecrementLazyKeepaliveCount is called when no calls to KeepaliveImpulse
@@ -459,8 +462,11 @@ void ProcessManager::OnKeepaliveImpulseCheck() {
        ++i) {
     if (i->second.previous_keepalive_impulse && !i->second.keepalive_impulse) {
       DecrementLazyKeepaliveCount(i->first);
-      if (!keepalive_impulse_decrement_callback_for_testing_.is_null())
-        keepalive_impulse_decrement_callback_for_testing_.Run(i->first);
+      if (!keepalive_impulse_decrement_callback_for_testing_.is_null()) {
+        ImpulseCallbackForTesting callback_may_clear_callbacks_reentrantly =
+          keepalive_impulse_decrement_callback_for_testing_;
+        callback_may_clear_callbacks_reentrantly.Run(i->first);
+      }
     }
 
     i->second.previous_keepalive_impulse = i->second.keepalive_impulse;