Declarative Web Request: Only register default web request rules registries if needed.

Currently we create default web request rules registries for the declarative web
request API even if the API is not available to the current environment. This is
wasteful and detrimental to performance, since the initial registry load blocks
the first network request (tracked via Extensions.NetworkDelayRegistryLoad UMA).

This CL changes RulesRegistryService so that the default WebRequestRulesRegistry
is registered only if the declarative web request API is avaialble to the
current environment. This, for example, means that no default web request rule
registries would be created on the stable channel. Rules registries
corresponding to webviews and the default content rules registry would still be
created.

This also helps fix issue 777717 and renderer cache is not cleared redundantly
on each extension load/unload/uninstall when the API is not available.

BUG=693243, 777717

Change-Id: I7384fed71a86aea3f5cc8d2eafd1445b439dd76a
Reviewed-on: https://chromium-review.googlesource.com/1072497Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563051}

chrome/browser/apps/guest_view/web_view_browsertest.cc

@@ -50,6 +50,8 @@
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/guest_view_manager_factory.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
+#include "components/version_info/channel.h"
+#include "components/version_info/version_info.h"
 #include "components/viz/common/features.h"
 #include "content/public/browser/ax_event_notification_details.h"
 #include "content/public/browser/browser_thread.h"
@@ -84,6 +86,7 @@
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extensions_client.h"
+#include "extensions/common/features/feature_channel.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "media/base/media_switches.h"
 #include "net/dns/mock_host_resolver.h"
@@ -3319,12 +3322,29 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestZoomBeforeNavigation) {
   TestHelper("testZoomBeforeNavigation", "web_view/shim", NO_TEST_SERVER);
 }
 
+// Test fixture to run the test on multiple channels.
+class WebViewChannelTest
+    : public WebViewTest,
+      public testing::WithParamInterface<version_info::Channel> {
+ public:
+  WebViewChannelTest() : channel_(GetParam()) {}
+
+ private:
+  extensions::ScopedCurrentChannel channel_;
+  DISALLOW_COPY_AND_ASSIGN(WebViewChannelTest);
+};
+
 // This test verify that the set of rules registries of a webview will be
 // removed from RulesRegistryService after the webview is gone.
 // http://crbug.com/438327
-IN_PROC_BROWSER_TEST_F(
-    WebViewTest,
+IN_PROC_BROWSER_TEST_P(
+    WebViewChannelTest,
     DISABLED_Shim_TestRulesRegistryIDAreRemovedAfterWebViewIsGone) {
+  ASSERT_EQ(extensions::GetCurrentChannel(), GetParam());
+  SCOPED_TRACE(
+      base::StringPrintf("Testing Channel %s",
+                         version_info::GetChannelString(GetParam()).c_str()));
+
   LoadAppWithGuest("web_view/rules_registry");
 
   content::WebContents* embedder_web_contents = GetEmbedderWebContents();
@@ -3348,12 +3368,12 @@ IN_PROC_BROWSER_TEST_F(
   extensions::RulesRegistryService* registry_service =
       extensions::RulesRegistryService::Get(profile);
   extensions::TestRulesRegistry* rules_registry =
-      new extensions::TestRulesRegistry(
-          content::BrowserThread::UI, "ui", rules_registry_id);
+      new extensions::TestRulesRegistry(content::BrowserThread::UI, "ui",
+                                        rules_registry_id);
   registry_service->RegisterRulesRegistry(base::WrapRefCounted(rules_registry));
 
-  EXPECT_TRUE(registry_service->GetRulesRegistry(
-      rules_registry_id, "ui").get());
+  EXPECT_TRUE(
+      registry_service->GetRulesRegistry(rules_registry_id, "ui").get());
 
   // Kill the embedder's render process, so the webview will go as well.
   base::Process process = base::Process::DeprecatedGetProcessFromHandle(
@@ -3364,12 +3384,17 @@ IN_PROC_BROWSER_TEST_F(
   process.Terminate(0, false);
   observer->WaitForEmbedderRenderProcessTerminate();
 
-  EXPECT_FALSE(registry_service->GetRulesRegistry(
-      rules_registry_id, "ui").get());
+  EXPECT_FALSE(
+      registry_service->GetRulesRegistry(rules_registry_id, "ui").get());
 }
 
-IN_PROC_BROWSER_TEST_F(WebViewTest,
+IN_PROC_BROWSER_TEST_P(WebViewChannelTest,
                        Shim_WebViewWebRequestRegistryHasNoPersistentCache) {
+  ASSERT_EQ(extensions::GetCurrentChannel(), GetParam());
+  SCOPED_TRACE(
+      base::StringPrintf("Testing Channel %s",
+                         version_info::GetChannelString(GetParam()).c_str()));
+
   LoadAppWithGuest("web_view/rules_registry");
 
   content::WebContents* guest_web_contents = GetGuestWebContents();
@@ -3388,9 +3413,11 @@ IN_PROC_BROWSER_TEST_F(WebViewTest,
 
   // Get an existing registered rule for the guest.
   extensions::RulesRegistry* registry =
-      registry_service->GetRulesRegistry(
-          rules_registry_id,
-          extensions::declarative_webrequest_constants::kOnRequest).get();
+      registry_service
+          ->GetRulesRegistry(
+              rules_registry_id,
+              extensions::declarative_webrequest_constants::kOnRequest)
+          .get();
 
   ASSERT_TRUE(registry);
   ASSERT_TRUE(registry->rules_cache_delegate_for_testing());
@@ -3398,6 +3425,11 @@ IN_PROC_BROWSER_TEST_F(WebViewTest,
             registry->rules_cache_delegate_for_testing()->type());
 }
 
+INSTANTIATE_TEST_CASE_P(,
+                        WebViewChannelTest,
+                        testing::Values(version_info::Channel::UNKNOWN,
+                                        version_info::Channel::STABLE));
+
 // This test verifies that webview.contentWindow works inside an iframe.
 IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestWebViewInsideFrame) {
   LoadAppWithGuest("web_view/inside_iframe");

chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc

@@ -10,12 +10,18 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/version_info/channel.h"
+#include "components/version_info/version_info.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/browser/api/declarative/test_rules_registry.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_constants.h"
+#include "extensions/common/api/declarative/declarative_constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/features/feature.h"
+#include "extensions/common/features/feature_channel.h"
+#include "extensions/common/features/feature_provider.h"
 #include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -124,4 +130,49 @@ TEST_F(RulesRegistryServiceTest, TestConstructionAndMultiThreading) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(RulesRegistryServiceTest, DefaultRulesRegistryRegistered) {
+  struct {
+    version_info::Channel channel;
+    bool expect_api_enabled;
+  } test_cases[] = {
+      {version_info::Channel::UNKNOWN, true},
+      {version_info::Channel::STABLE, false},
+  };
+
+  for (const auto& test_case : test_cases) {
+    SCOPED_TRACE(base::StringPrintf(
+        "Testing Channel %s",
+        version_info::GetChannelString(test_case.channel).c_str()));
+    ScopedCurrentChannel scoped_channel(test_case.channel);
+
+    ASSERT_EQ(test_case.expect_api_enabled,
+              FeatureProvider::GetAPIFeature("declarativeWebRequest")
+                  ->IsAvailableToEnvironment()
+                  .is_available());
+
+    TestingProfile profile;
+    RulesRegistryService registry_service(&profile);
+
+    // The default web request rules registry should only be created if the API
+    // is enabled.
+    EXPECT_EQ(
+        test_case.expect_api_enabled,
+        registry_service
+                .GetRulesRegistry(RulesRegistryService::kDefaultRulesRegistryID,
+                                  declarative_webrequest_constants::kOnRequest)
+                .get() != nullptr);
+
+    // Content rules registry should always be created.
+    EXPECT_TRUE(registry_service.GetRulesRegistry(
+        RulesRegistryService::kDefaultRulesRegistryID,
+        declarative_content_constants::kOnPageChanged));
+    EXPECT_TRUE(registry_service.content_rules_registry());
+
+    // Rules registries for web views should always be created.
+    const int kWebViewRulesRegistryID = 1;
+    EXPECT_TRUE(registry_service.GetRulesRegistry(
+        kWebViewRulesRegistryID, declarative_webrequest_constants::kOnRequest));
+  }
+}
+
 }  // namespace extensions

extensions/browser/api/declarative/rules_registry_service.cc

@@ -20,6 +20,8 @@
 #include "extensions/browser/api/web_request/web_request_api.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/features/feature.h"
+#include "extensions/common/features/feature_provider.h"
 
 namespace extensions {
 
@@ -194,9 +196,17 @@ void RulesRegistryService::EnsureDefaultRulesRegistriesRegistered() {
       RulesRegistryKey(declarative_webrequest_constants::kOnRequest,
                        kDefaultRulesRegistryID)));
 
-  // Persist the cache since it pertains to regular pages (i.e. not webviews).
-  RegisterWebRequestRulesRegistry(kDefaultRulesRegistryID,
-                                  RulesCacheDelegate::Type::kPersistent);
+  // Only register the default web request rules registry if the
+  // declarativeWebRequest API is enabled. See crbug.com/693243.
+  const bool is_api_enabled =
+      FeatureProvider::GetAPIFeature("declarativeWebRequest")
+          ->IsAvailableToEnvironment()
+          .is_available();
+  if (is_api_enabled) {
+    // Persist the cache since it pertains to regular pages (i.e. not webviews).
+    RegisterWebRequestRulesRegistry(kDefaultRulesRegistryID,
+                                    RulesCacheDelegate::Type::kPersistent);
+  }
 
   // Create the ContentRulesRegistry.
   DCHECK(!content_rules_registry_);