Extensions: Make ExtensionWebRequestEventRouter leaky.

extensions/browser/api/web_request/web_request_api.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 "extensions/browser/api/web_request/web_request_api.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 383 matching lines @@
   // Note that details.event_name includes the sub-event details (e.g. "/123").
   // TODO(fsamuel): <webview> events will not be removed through this code path.
   // <webview> events will be removed in RemoveWebViewEventListeners. Ideally,
   // this code should be decoupled from extensions, we should use the host ID
   // instead, and not have two different code paths. This is a huge undertaking
   // unfortunately, so we'll resort to two code paths for now.
   //
   // 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(
           &ExtensionWebRequestEventRouter::RemoveEventListener,
           base::Unretained(ExtensionWebRequestEventRouter::GetInstance()), id,
           false /* not strict */));
 }
 
 // Represents a single unique listener to an event, along with whatever filter
 // parameters and extra_info_spec were specified at the time the listener was
@@ skipping 142 matching lines @@
 
 ExtensionWebRequestEventRouter::RequestFilter::~RequestFilter() {
 }
 
 //
 // ExtensionWebRequestEventRouter
 //
 
 // 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()
     : request_time_tracker_(new ExtensionWebRequestTimeTracker),
       web_request_event_router_delegate_(
           ExtensionsAPIClient::Get()->CreateWebRequestEventRouterDelegate()) {}
 
-ExtensionWebRequestEventRouter::~ExtensionWebRequestEventRouter() {
-}
-
 void ExtensionWebRequestEventRouter::RegisterRulesRegistry(
     void* browser_context,
     int rules_registry_id,
     scoped_refptr<WebRequestRulesRegistry> rules_registry) {
   RulesRegistryKey key(browser_context, rules_registry_id);
   if (rules_registry.get())
     rules_registries_[key] = rules_registry;
   else
     rules_registries_.erase(key);
 }
@@ skipping 514 matching lines @@
   // (eg when created by a URLFetcher instead of the ResourceDispatcherHost).
   const content::ResourceRequestInfo* info =
       content::ResourceRequestInfo::ForRequest(request);
   if (content::IsBrowserSideNavigationEnabled() && info &&
       IsResourceTypeFrame(info->GetResourceType())) {
     DCHECK(navigation_ui_data);
     event_details->SetFrameData(navigation_ui_data->frame_data());
     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) {
     BlockedRequest& blocked_request = blocked_requests_[request->identifier()];
     blocked_request.request = request;
     blocked_request.is_incognito |= IsIncognitoBrowserContext(browser_context);
     blocked_request.num_handlers_blocking += num_handlers_blocking;
     blocked_request.blocking_time = base::Time::Now();
     return true;
   }
@@ skipping 809 matching lines @@
 
   // The following block is experimentally enabled and its impact on load time
   // logged with UMA Extensions.NetworkDelayRegistryLoad. crbug.com/175961
   for (auto it : relevant_registries) {
     WebRequestRulesRegistry* rules_registry = it.first;
     if (rules_registry->ready().is_signaled())
       continue;
 
     // 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++;
     blocked_request.request = request;
     blocked_request.is_incognito |= IsIncognitoBrowserContext(browser_context);
     blocked_request.blocking_time = base::Time::Now();
     blocked_request.original_response_headers = original_response_headers;
     blocked_request.extension_info_map = extension_info_map;
     return true;
   }
@@ skipping 433 matching lines @@
   // Since EventListeners are segmented by browser_context, check that
   // last, as it is exceedingly unlikely to be different.
   return extension_id == that.extension_id &&
          sub_event_name == that.sub_event_name &&
          web_view_instance_id == that.web_view_instance_id &&
          embedder_process_id == that.embedder_process_id &&
          browser_context == that.browser_context;
 }
 
 }  // namespace extensions