[Extensions] Guarantee API functions have unique histogram values

The histogram values for an extension function are used to report
API call activity and related metrics for extension functions. Each
extension function should have a unique histogram value, but some of
them were erroneously copy-pasted. This leads to incorrect metrics, both
with not reporting one function and over-reporting another.

Fix the instances where the histogram values were duplicated, and add a
browser test that ensures each function has a unique histogram value.

Bug: 827601

Change-Id: Ic44519ca83fe0cf4b3a1896a16232f6400b6c79d
Reviewed-on: https://chromium-review.googlesource.com/988227
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548654}

chrome/browser/extensions/api/automation_internal/automation_internal_api.h

@@ -56,7 +56,8 @@ class AutomationInternalPerformActionFunction
 
 class AutomationInternalEnableFrameFunction : public UIThreadExtensionFunction {
   DECLARE_EXTENSION_FUNCTION("automationInternal.enableFrame",
-                             AUTOMATIONINTERNAL_PERFORMACTION)
+                             AUTOMATIONINTERNAL_ENABLEFRAME)
+
  protected:
   ~AutomationInternalEnableFrameFunction() override {}
 
@@ -76,7 +77,7 @@ class AutomationInternalEnableDesktopFunction
 class AutomationInternalQuerySelectorFunction
     : public UIThreadExtensionFunction {
   DECLARE_EXTENSION_FUNCTION("automationInternal.querySelector",
-                             AUTOMATIONINTERNAL_ENABLEDESKTOP)
+                             AUTOMATIONINTERNAL_QUERYSELECTOR)
 
  public:
   typedef base::Callback<void(const std::string& error,

chrome/browser/extensions/api/debugger/debugger_api.h

@@ -89,7 +89,7 @@ class DebuggerSendCommandFunction : public DebuggerFunction {
 // Implements the debugger.getTargets() extension function.
 class DebuggerGetTargetsFunction : public DebuggerFunction {
  public:
-  DECLARE_EXTENSION_FUNCTION("debugger.getTargets", DEBUGGER_ATTACH)
+  DECLARE_EXTENSION_FUNCTION("debugger.getTargets", DEBUGGER_GETTARGETS)
 
   DebuggerGetTargetsFunction();
 

chrome/browser/extensions/api/notifications/notifications_api.h

@@ -135,7 +135,7 @@ class NotificationsGetPermissionLevelFunction
 
  private:
   DECLARE_EXTENSION_FUNCTION("notifications.getPermissionLevel",
-                             NOTIFICATIONS_GET_ALL)
+                             NOTIFICATIONS_GETPERMISSIONLEVEL)
 };
 
 }  // namespace extensions

chrome/browser/extensions/extension_function_registration_test.cc

+// Copyright 2018 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 "chrome/browser/extensions/extension_browsertest.h"
+#include "extensions/browser/extension_function_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/one_shot_event.h"
+
+namespace extensions {
+
+using ExtensionFunctionRegistrationTest = ExtensionBrowserTest;
+
+// Test that all functions are registered with unique names, histogram values,
+// and factories. This is a browser test (rather than a unit test) to (help)
+// ensure that all the optional factories and services are indeed instantiated.
+IN_PROC_BROWSER_TEST_F(ExtensionFunctionRegistrationTest,
+                       CheckForDuplicateEntries) {
+  // Verify the ExtensionSystem is ready (and thus all extension functions
+  // registered) before checking.
+  base::RunLoop run_loop;
+  ExtensionSystem::Get(profile())->ready().Post(FROM_HERE,
+                                                run_loop.QuitClosure());
+  run_loop.Run();
+
+  const ExtensionFunctionRegistry::FactoryMap& factories =
+      ExtensionFunctionRegistry::GetInstance().GetFactoriesForTesting();
+  // Sanity check: Many, many functions should have been registered.
+  EXPECT_GT(factories.size(), 500u);
+
+  std::set<std::string> seen_names;
+  std::set<functions::HistogramValue> seen_histograms;
+  for (const auto& key_value : factories) {
+    const ExtensionFunctionRegistry::FactoryEntry& entry = key_value.second;
+    SCOPED_TRACE(entry.function_name_);
+    EXPECT_TRUE(seen_names.insert(entry.function_name_).second);
+    // NOTE: We explicitly don't check the factory here. On certain platforms
+    // with enough compiler optimization, the templated factories are re-used
+    // for different functions.
+    // EXPECT_TRUE(seen_factories.insert(entry.factory_).second);
+
+    // The chrome.test API uses an "unknown" histogram value, but should be the
+    // only API that does.
+    if (entry.histogram_value_ == functions::UNKNOWN) {
+      EXPECT_TRUE(base::StartsWith(entry.function_name_, "test.",
+                                   base::CompareCase::SENSITIVE));
+    } else {
+      EXPECT_TRUE(seen_histograms.insert(entry.histogram_value_).second);
+    }
+  }
+}
+
+}  // namespace extensions

chrome/test/BUILD.gn

@@ -1260,6 +1260,7 @@ test("browser_tests") {
         "../browser/extensions/extension_disabled_ui_browsertest.cc",
         "../browser/extensions/extension_dom_clipboard_apitest.cc",
         "../browser/extensions/extension_fileapi_apitest.cc",
+        "../browser/extensions/extension_function_registration_test.cc",
         "../browser/extensions/extension_function_test_utils.cc",
         "../browser/extensions/extension_function_test_utils.h",
         "../browser/extensions/extension_functional_browsertest.cc",

extensions/browser/extension_function_histogram_value.h

@@ -1295,6 +1295,10 @@ enum HistogramValue {
   DECLARATIVENETREQUEST_REMOVEWHITELISTEDPAGES,
   DECLARATIVENETREQUEST_GETWHITELISTEDPAGES,
   DEVELOPERPRIVATE_INSTALLDROPPEDFILE,
+  AUTOMATIONINTERNAL_ENABLEFRAME,
+  AUTOMATIONINTERNAL_QUERYSELECTOR,
+  DEBUGGER_GETTARGETS,
+  NOTIFICATIONS_GETPERMISSIONLEVEL,
   // Last entry: Add new entries above, then run:
   // python tools/metrics/histograms/update_extension_histograms.py
   ENUM_BOUNDARY

extensions/browser/extension_function_registry.h

@@ -42,6 +42,7 @@ class ExtensionFunctionRegistry {
     extensions::functions::HistogramValue histogram_value_ =
         extensions::functions::UNKNOWN;
   };
+  using FactoryMap = std::map<std::string, FactoryEntry>;
 
   static ExtensionFunctionRegistry& GetInstance();
   ExtensionFunctionRegistry();
@@ -63,8 +64,9 @@ class ExtensionFunctionRegistry {
                           T::histogram_value()));
   }
 
+  const FactoryMap& GetFactoriesForTesting() const { return factories_; }
+
  private:
-  typedef std::map<std::string, FactoryEntry> FactoryMap;
   FactoryMap factories_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionFunctionRegistry);

tools/metrics/histograms/enums.xml

@@ -14968,6 +14968,10 @@ Called by update_net_error_codes.py.-->
   <int value="1232" label="DECLARATIVENETREQUEST_REMOVEWHITELISTEDPAGES"/>
   <int value="1233" label="DECLARATIVENETREQUEST_GETWHITELISTEDPAGES"/>
   <int value="1234" label="DEVELOPERPRIVATE_INSTALLDROPPEDFILE"/>
+  <int value="1235" label="AUTOMATIONINTERNAL_ENABLEFRAME"/>
+  <int value="1236" label="AUTOMATIONINTERNAL_QUERYSELECTOR"/>
+  <int value="1237" label="DEBUGGER_GETTARGETS"/>
+  <int value="1238" label="NOTIFICATIONS_GETPERMISSIONLEVEL"/>
 </enum>
 
 <enum name="ExtensionIconState">