Extensions: Make ExtensionWebRequestEventRouter leaky.

extensions/browser/api/web_request/web_request_api.cc

Index: extensions/browser/api/web_request/web_request_api.cc
diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc
index b81023248535d3064adaf563327f06568d444b64..e9d8ff2e52ea1e010e9776e34ceaf9af4fb097f8 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -401,6 +401,9 @@ void WebRequestAPI::OnListenerRemoved(const EventListenerInfo& details) {
   // Note that details.event_name is actually the sub_event_name!
   ExtensionWebRequestEventRouter::EventListener::ID id(
       details.browser_context, details.extension_id, details.event_name, 0, 0);
+
+  // This Unretained is safe because the ExtensionWebRequestEventRouter
+  // singleton is leaked.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(
@@ -563,7 +566,8 @@ ExtensionWebRequestEventRouter::RequestFilter::~RequestFilter() {
 
 // static
 ExtensionWebRequestEventRouter* ExtensionWebRequestEventRouter::GetInstance() {
-  return base::Singleton<ExtensionWebRequestEventRouter>::get();
+  CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());

[Wez, 2017/03/01 17:28:16]

Will this have the correct behaviour with respect to instance state across
tests?

[karandeepb, 2017/03/01 18:07:21]

Yeah state will be carried over for unit tests run in the same process. Isn't it
the same for base::Singleton though? 

[Wez, 2017/03/01 18:09:47]

My recollection was that Singleton and LazyInstance *will* be torn-down by the
test-specific AtExitManager (maybe ShadowAtExitManager, but I may be thinking of
something else)?

[karandeepb, 2017/03/01 19:17:51]

Yeah so there's the ShadowingAtExitManager which you can add to your tests/test
suite, which will call the registered AtExit callbacks whenever it is itself
destroyed, hence destroying any Singletons. At least for the tests in
web_request_api_unittest.cc, this wasn't the case. (There was no
ShadowingAtExitManager). 

Also, even if I were to use a leaky base::Singleton, I think the problem will
still be there since it won't register an AtExit callback for its destruction.
(Maybe it should when running tests?) I guess this is a problem for most/all
leaky singletons and requires explicit handling in tests itself. In case we do
start seeing some flakes, this can probably be revisited.

+  return &instance;
 }
 
 ExtensionWebRequestEventRouter::ExtensionWebRequestEventRouter()
@@ -571,9 +575,6 @@ ExtensionWebRequestEventRouter::ExtensionWebRequestEventRouter()
       web_request_event_router_delegate_(
           ExtensionsAPIClient::Get()->CreateWebRequestEventRouterDelegate()) {}
 
-ExtensionWebRequestEventRouter::~ExtensionWebRequestEventRouter() {
-}
-
 void ExtensionWebRequestEventRouter::RegisterRulesRegistry(
     void* browser_context,
     int rules_registry_id,
@@ -1108,9 +1109,12 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
     DispatchEventToListeners(browser_context, std::move(listeners_to_dispatch),
                              std::move(event_details));
   } else {
-    event_details.release()->DetermineFrameDataOnIO(base::Bind(
-        &ExtensionWebRequestEventRouter::DispatchEventToListeners, AsWeakPtr(),
-        browser_context, base::Passed(&listeners_to_dispatch)));
+    // This Unretained is safe because the ExtensionWebRequestEventRouter
+    // singleton is leaked.
+    event_details.release()->DetermineFrameDataOnIO(
+        base::Bind(&ExtensionWebRequestEventRouter::DispatchEventToListeners,
+                   base::Unretained(this), browser_context,
+                   base::Passed(&listeners_to_dispatch)));
   }
 
   if (num_handlers_blocking > 0) {
@@ -1940,10 +1944,12 @@ bool ExtensionWebRequestEventRouter::ProcessDeclarativeRules(
 
     // The rules registry is still loading. Block this request until it
     // finishes.
+    // This Unretained is safe because the ExtensionWebRequestEventRouter
+    // singleton is leaked.
     rules_registry->ready().Post(
         FROM_HERE,
         base::Bind(&ExtensionWebRequestEventRouter::OnRulesRegistryReady,
-                   AsWeakPtr(), browser_context, event_name,
+                   base::Unretained(this), browser_context, event_name,
                    request->identifier(), request_stage));
     BlockedRequest& blocked_request = blocked_requests_[request->identifier()];
     blocked_request.num_handlers_blocking++;