[Extensions Performance] Only record action updates for defaults

Extensions can update the default settings of browser actions (such as
badge text, popup url, icon, and more). These settings are persisted in
storage across runs. However, currently the logic to persist these and
rewrite the values in storage is being triggered even if the extension
is only updating a specific tab's value (in which case it will just
rewrite all the same values). This is costly, as it requires both thread
hops and disk IO.

As a first step and low-hanging fruit, ensure that we don't try to write
the default values if the extension only updated the settings for a
specific tab (in which case we should know that nothing in the default
settings changed).

Bug: 505676
Change-Id: I612408740e88b76b234bf0d38433ab716485829b
Reviewed-on: https://chromium-review.googlesource.com/768307
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522152}

chrome/browser/extensions/api/extension_action/extension_action_apitest.cc

+// Copyright 2017 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 <map>
+#include <string>
+
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/extensions/browsertest_util.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/test_extension_dir.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "extensions/browser/state_store.h"
+#include "extensions/test/extension_test_message_listener.h"
+
+namespace extensions {
+namespace {
+
+// A helper class to track StateStore changes.
+class TestStateStoreObserver : public StateStore::TestObserver {
+ public:
+  TestStateStoreObserver(content::BrowserContext* context,
+                         const std::string& extension_id)
+      : extension_id_(extension_id), scoped_observer_(this) {
+    scoped_observer_.Add(ExtensionSystem::Get(context)->state_store());
+  }
+  ~TestStateStoreObserver() override {}
+
+  void WillSetExtensionValue(const std::string& extension_id,
+                             const std::string& key) override {
+    if (extension_id == extension_id_)
+      ++updated_values_[key];
+  }
+
+  int CountForKey(const std::string& key) const {
+    auto iter = updated_values_.find(key);
+    return iter == updated_values_.end() ? 0 : iter->second;
+  }
+
+ private:
+  std::string extension_id_;
+  std::map<std::string, int> updated_values_;
+
+  ScopedObserver<StateStore, StateStore::TestObserver> scoped_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestStateStoreObserver);
+};
+
+// A helper class to observe ExtensionActionAPI changes.
+class TestExtensionActionAPIObserver : public ExtensionActionAPI::Observer {
+ public:
+  TestExtensionActionAPIObserver(content::BrowserContext* context,
+                                 const std::string& extension_id)
+      : extension_id_(extension_id), scoped_observer_(this) {
+    scoped_observer_.Add(ExtensionActionAPI::Get(context));
+  }
+  ~TestExtensionActionAPIObserver() override {}
+
+  void OnExtensionActionUpdated(
+      ExtensionAction* extension_action,
+      content::WebContents* web_contents,
+      content::BrowserContext* browser_context) override {
+    if (extension_action->extension_id() == extension_id_) {
+      last_web_contents_ = web_contents;
+      run_loop_.QuitWhenIdle();
+    }
+  }
+
+  const content::WebContents* last_web_contents() const {
+    return last_web_contents_;
+  }
+
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  content::WebContents* last_web_contents_ = nullptr;
+  std::string extension_id_;
+  base::RunLoop run_loop_;
+  ScopedObserver<ExtensionActionAPI, ExtensionActionAPI::Observer>
+      scoped_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestExtensionActionAPIObserver);
+};
+
+}  // namespace
+
+using ExtensionActionAPITest = ExtensionApiTest;
+
+// Check that updating the browser action badge for a specific tab id does not
+// cause a disk write (since we only persist the defaults).
+IN_PROC_BROWSER_TEST_F(ExtensionActionAPITest, TestNoUnnecessaryIO) {
+  ExtensionTestMessageListener ready_listener("ready", false);
+
+  TestExtensionDir test_dir;
+  test_dir.WriteManifest(
+      R"({
+           "name": "Extension",
+           "description": "An extension",
+           "manifest_version": 2,
+           "version": "0.1",
+           "browser_action": {},
+           "background": { "scripts": ["background.js"] }
+         })");
+  test_dir.WriteFile(FILE_PATH_LITERAL("background.js"),
+                     "chrome.test.sendMessage('ready');");
+
+  const Extension* extension = LoadExtension(test_dir.UnpackedPath());
+  ASSERT_TRUE(extension);
+  ASSERT_TRUE(ready_listener.WaitUntilSatisfied());
+
+  // The script template to update the browser action.
+  constexpr char kUpdate[] =
+      R"(chrome.browserAction.setBadgeText(%s);
+         domAutomationController.send('pass');)";
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  int tab_id = SessionTabHelper::IdForTab(web_contents);
+  constexpr char kBrowserActionKey[] = "browser_action";
+  TestStateStoreObserver test_state_store_observer(profile(), extension->id());
+
+  {
+    TestExtensionActionAPIObserver test_api_observer(profile(),
+                                                     extension->id());
+    // First, update a specific tab.
+    std::string update_options =
+        base::StringPrintf("{text: 'New Text', tabId: %d}", tab_id);
+    EXPECT_EQ("pass", browsertest_util::ExecuteScriptInBackgroundPage(
+                          profile(), extension->id(),
+                          base::StringPrintf(kUpdate, update_options.c_str())));
+    test_api_observer.Wait();
+
+    // The action update should be associated with the specific tab.
+    EXPECT_EQ(web_contents, test_api_observer.last_web_contents());
+    // Since this was only updating a specific tab, this should *not* result in
+    // a StateStore write. We should only write to the StateStore with new
+    // default values.
+    EXPECT_EQ(0, test_state_store_observer.CountForKey(kBrowserActionKey));
+  }
+
+  {
+    TestExtensionActionAPIObserver test_api_observer(profile(),
+                                                     extension->id());
+    // Next, update the default badge text.
+    EXPECT_EQ("pass",
+              browsertest_util::ExecuteScriptInBackgroundPage(
+                  profile(), extension->id(),
+                  base::StringPrintf(kUpdate, "{text: 'Default Text'}")));
+    test_api_observer.Wait();
+    // The action update should not be associated with a specific tab.
+    EXPECT_EQ(nullptr, test_api_observer.last_web_contents());
+
+    // This *should* result in a StateStore write, since we persist the default
+    // state of the extension action.
+    EXPECT_EQ(1, test_state_store_observer.CountForKey(kBrowserActionKey));
+  }
+}
+
+}  // namespace extensions

chrome/browser/extensions/extension_action_storage_manager.cc

@@ -233,9 +233,15 @@ void ExtensionActionStorageManager::OnExtensionActionUpdated(
     ExtensionAction* extension_action,
     content::WebContents* web_contents,
     content::BrowserContext* browser_context) {
+  // This is an update to the default settings of the action iff |web_contents|
+  // is null. We only persist the default settings to disk, since per-tab
+  // settings can't be persisted across browser sessions.
+  bool for_default_tab = !web_contents;
   if (browser_context_ == browser_context &&
-      extension_action->action_type() == ActionInfo::TYPE_BROWSER)
+      extension_action->action_type() == ActionInfo::TYPE_BROWSER &&
+      for_default_tab) {
     WriteToStorage(extension_action);
+  }
 }
 
 void ExtensionActionStorageManager::OnExtensionActionAPIShuttingDown() {

chrome/test/BUILD.gn

@@ -1105,6 +1105,7 @@ test("browser_tests") {
         "../browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc",
         "../browser/extensions/api/extension_action/browser_action_apitest.cc",
         "../browser/extensions/api/extension_action/browser_action_browsertest.cc",
+        "../browser/extensions/api/extension_action/extension_action_apitest.cc",
         "../browser/extensions/api/extension_action/page_action_apitest.cc",
         "../browser/extensions/api/feedback_private/feedback_browsertest.cc",
         "../browser/extensions/api/file_system/file_system_apitest.cc",

extensions/browser/state_store.cc

@@ -114,6 +114,9 @@ void StateStore::GetExtensionValue(const std::string& extension_id,
 void StateStore::SetExtensionValue(const std::string& extension_id,
                                    const std::string& key,
                                    std::unique_ptr<base::Value> value) {
+  for (TestObserver& observer : observers_)
+    observer.WillSetExtensionValue(extension_id, key);
+
   task_queue_->InvokeWhenReady(
       base::Bind(&ValueStoreFrontend::Set, base::Unretained(store_.get()),
                  GetFullKey(extension_id, key), base::Passed(&value)));
@@ -126,6 +129,14 @@ void StateStore::RemoveExtensionValue(const std::string& extension_id,
                                           GetFullKey(extension_id, key)));
 }
 
+void StateStore::AddObserver(TestObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void StateStore::RemoveObserver(TestObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 bool StateStore::IsInitialized() const {
   return task_queue_->ready();
 }

extensions/browser/state_store.h

@@ -9,7 +9,9 @@
 #include <string>
 
 #include "base/files/file_path.h"
+#include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/scoped_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -32,6 +34,13 @@ class StateStore : public base::SupportsWeakPtr<StateStore>,
  public:
   typedef ValueStoreFrontend::ReadCallback ReadCallback;
 
+  class TestObserver {
+   public:
+    virtual ~TestObserver() {}
+    virtual void WillSetExtensionValue(const std::string& extension_id,
+                                       const std::string& key) = 0;
+  };
+
   // If |deferred_load| is true, we won't load the database until the first
   // page has been loaded.
   StateStore(content::BrowserContext* context,
@@ -70,6 +79,9 @@ class StateStore : public base::SupportsWeakPtr<StateStore>,
   // Return whether or not the StateStore has initialized itself.
   bool IsInitialized() const;
 
+  void AddObserver(TestObserver* observer);
+  void RemoveObserver(TestObserver* observer);
+
  private:
   class DelayedTaskQueue;
 
@@ -106,10 +118,14 @@ class StateStore : public base::SupportsWeakPtr<StateStore>,
   // Keeps track of tasks we have delayed while starting up.
   std::unique_ptr<DelayedTaskQueue> task_queue_;
 
+  base::ObserverList<TestObserver> observers_;
+
   content::NotificationRegistrar registrar_;
 
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(StateStore);
 };
 
 }  // namespace extensions