[Extensions] Test "action" key property persistence

For browser actions, certain properties are persisted, while others are
not. (By contrast, page actions do not persist any properties).

Since we're introducing the generic "action" key, we need to provide
defined behavior for persistence. In this case, we default to *not*
persisting values. This is consistent across properties (unlike
browser action), and it is easier to add in support later if we
evaluate that it would be worthwhile.

Add a browsertest that covers both browser action values and the
new "action" values.

Bug: 1110156, 893373
Change-Id: Icc4472599e52e0309cab0b3704ea9394865787de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439975Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815847}

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

@@ -184,6 +184,14 @@ class ActionTestHelper {
   DISALLOW_COPY_AND_ASSIGN(ActionTestHelper);
 };
 
+// Forces a flush of the StateStore, where action state is persisted.
+void FlushStateStore(Profile* profile) {
+  base::RunLoop run_loop;
+  ExtensionSystem::Get(profile)->state_store()->FlushForTesting(
+      run_loop.QuitWhenIdleClosure());
+  run_loop.Run();
+}
+
 }  // namespace
 
 using ExtensionActionAPITest = ExtensionApiTest;
@@ -638,6 +646,78 @@ IN_PROC_BROWSER_TEST_P(MultiActionAPITest,
   EXPECT_EQ("1", foo);
 }
 
+using ActionAndBrowserActionAPITest = MultiActionAPITest;
+
+// Tests whether action values persist across sessions.
+// Note: Since pageActions are only applicable on a specific tab, this test
+// doesn't apply to them.
+IN_PROC_BROWSER_TEST_P(ActionAndBrowserActionAPITest, PRE_ValuesArePersisted) {
+  const char* dir_name = nullptr;
+  switch (GetParam()) {
+    case ActionInfo::TYPE_ACTION:
+      dir_name = "extension_action/action_persistence";
+      break;
+    case ActionInfo::TYPE_BROWSER:
+      dir_name = "extension_action/browser_action_persistence";
+      break;
+    case ActionInfo::TYPE_PAGE:
+      NOTREACHED();
+      break;
+  }
+  // Load up an extension, which then modifies the popup, title, and badge text
+  // of the action. We need to use a "real" extension on disk here (rather than
+  // a TestExtensionDir owned by the test fixture), because it needs to persist
+  // to the next test.
+  ResultCatcher catcher;
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII(dir_name));
+  ASSERT_TRUE(extension);
+  ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
+
+  // Verify the values were modified.
+  auto* action_manager = ExtensionActionManager::Get(profile());
+  ExtensionAction* action = action_manager->GetExtensionAction(*extension);
+  EXPECT_EQ(extension->GetResourceURL("modified_popup.html"),
+            action->GetPopupUrl(ExtensionAction::kDefaultTabId));
+  EXPECT_EQ("modified title", action->GetTitle(ExtensionAction::kDefaultTabId));
+  EXPECT_EQ("custom badge text",
+            action->GetExplicitlySetBadgeText(ExtensionAction::kDefaultTabId));
+
+  // We flush the state store to ensure the modified state is correctly stored
+  // on-disk (which could otherwise be potentially racy).
+  FlushStateStore(profile());
+}
+
+IN_PROC_BROWSER_TEST_P(ActionAndBrowserActionAPITest, ValuesArePersisted) {
+  const Extension* extension = GetSingleLoadedExtension();
+  ASSERT_TRUE(extension);
+  EXPECT_EQ("Action persistence check", extension->name());
+
+  // The previous action states are read from the state store on start-up.
+  // Flushing it ensures that any pending tasks have run, and the action
+  // should be up-to-date.
+  FlushStateStore(profile());
+
+  auto* action_manager = ExtensionActionManager::Get(profile());
+  ExtensionAction* action = action_manager->GetExtensionAction(*extension);
+
+  // Only browser actions - not generic actions - persist values.
+  bool expect_persisted_values = GetParam() == ActionInfo::TYPE_BROWSER;
+
+  std::string expected_badge_text =
+      expect_persisted_values ? "custom badge text" : "";
+
+  EXPECT_EQ(expected_badge_text,
+            action->GetExplicitlySetBadgeText(ExtensionAction::kDefaultTabId));
+
+  // Due to https://crbug.com/1110156, action values with defaults specified in
+  // the manifest - like popup and title - aren't persisted, even for browser
+  // actions.
+  EXPECT_EQ(extension->GetResourceURL("default_popup.html"),
+            action->GetPopupUrl(ExtensionAction::kDefaultTabId));
+  EXPECT_EQ("default title", action->GetTitle(ExtensionAction::kDefaultTabId));
+}
+
 // Tests setting the icon dynamically from the background page.
 IN_PROC_BROWSER_TEST_P(MultiActionAPICanvasTest, DynamicSetIcon) {
   constexpr char kManifestTemplate[] =
@@ -1177,6 +1257,11 @@ INSTANTIATE_TEST_SUITE_P(All,
                                          ActionInfo::TYPE_PAGE,
                                          ActionInfo::TYPE_BROWSER));
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         ActionAndBrowserActionAPITest,
+                         testing::Values(ActionInfo::TYPE_ACTION,
+                                         ActionInfo::TYPE_BROWSER));
+
 INSTANTIATE_TEST_SUITE_P(All,
                          MultiActionAPICanvasTest,
                          testing::Values(ActionInfo::TYPE_ACTION,

chrome/browser/extensions/extension_action_storage_manager.cc

@@ -233,7 +233,6 @@ void ExtensionActionStorageManager::OnExtensionActionUpdated(
   // 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;
-  // TODO(devlin): We should probably persist for TYPE_ACTION as well.
   if (browser_context_ == browser_context &&
       extension_action->action_type() == ActionInfo::TYPE_BROWSER &&
       for_default_tab) {

chrome/test/data/extensions/api_test/extension_action/action_persistence/background.js

+// Copyright 2020 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.
+
+chrome.storage.local.get('sentinel', (val) => {
+  if (val.sentinel !== undefined) {
+    // Note: We don't expect the event page to be invoked a second time,
+    // because it doesn't register for any relevant events. If it were, it would
+    // re-set the action properties, which would invalidate the text.
+    chrome.test.notifyFail('Unexpected Sentinel Value: ' + val.sentinel);
+    chrome.action.setTitle({title: 'FAILED'});
+    return;
+  }
+
+  chrome.storage.local.set({sentinel: true}, () => {
+    chrome.action.setPopup({popup: 'modified_popup.html'}, () => {
+      chrome.test.assertNoLastError();
+      chrome.action.setTitle({title: 'modified title'}, () => {
+        chrome.test.assertNoLastError();
+        chrome.action.setBadgeText({text: 'custom badge text'}, () => {
+          chrome.test.assertNoLastError();
+          chrome.test.notifyPass();
+        });
+      });
+    });
+  });
+});

chrome/test/data/extensions/api_test/extension_action/action_persistence/default_popup.html

+<!doctype html>
+<html></html>

chrome/test/data/extensions/api_test/extension_action/action_persistence/manifest.json

+{
+  "name": "Action persistence check",
+  "version": "0.1",
+  "manifest_version": 2,
+  "background": { "scripts": ["background.js"], "persistent": false },
+  "action": {
+    "default_popup": "default_popup.html",
+    "default_title": "default title"
+  },
+  "permissions": ["storage"]
+}

chrome/test/data/extensions/api_test/extension_action/action_persistence/modified_popup.html

+<!doctype html>
+<html></html>

chrome/test/data/extensions/api_test/extension_action/browser_action_persistence/background.js

+// Copyright 2020 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.
+
+chrome.storage.local.get('sentinel', (val) => {
+  if (val.sentinel !== undefined) {
+    // Note: We don't expect the event page to be invoked a second time,
+    // because it doesn't register for any relevant events. If it were, it would
+    // re-set the action properties, which would invalidate the text.
+    chrome.test.notifyFail('Unexpected Sentinel Value: ' + val.sentinel);
+    chrome.browserAction.setTitle({title: 'FAILED'});
+    return;
+  }
+
+  chrome.storage.local.set({sentinel: true}, () => {
+    chrome.browserAction.setPopup({popup: 'modified_popup.html'}, () => {
+      chrome.test.assertNoLastError();
+      chrome.browserAction.setTitle({title: 'modified title'}, () => {
+        chrome.test.assertNoLastError();
+        chrome.browserAction.setBadgeText({text: 'custom badge text'}, () => {
+          chrome.test.assertNoLastError();
+          chrome.test.notifyPass();
+        });
+      });
+    });
+  });
+});

chrome/test/data/extensions/api_test/extension_action/browser_action_persistence/default_popup.html

+<!doctype html>
+<html></html>

chrome/test/data/extensions/api_test/extension_action/browser_action_persistence/manifest.json

+{
+  "name": "Action persistence check",
+  "version": "0.1",
+  "manifest_version": 2,
+  "background": { "scripts": ["background.js"], "persistent": false },
+  "browser_action": {
+    "default_popup": "default_popup.html",
+    "default_title": "default title"
+  },
+  "permissions": ["storage"]
+}

chrome/test/data/extensions/api_test/extension_action/browser_action_persistence/modified_popup.html

+<!doctype html>
+<html></html>

extensions/browser/state_store.cc

@@ -135,6 +135,18 @@ void StateStore::RemoveObserver(TestObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void StateStore::FlushForTesting(base::OnceClosure flushed_callback) {
+  // Look up a key in the database. This serves as a roundtrip to the DB and
+  // back; the value of the key doesn't matter.
+  GetExtensionValue("fake_id", "fake_key",
+                    base::BindOnce(
+                        [](base::OnceClosure flushed_callback,
+                           std::unique_ptr<base::Value> ignored) {
+                          std::move(flushed_callback).Run();
+                        },
+                        std::move(flushed_callback)));
+}
+
 bool StateStore::IsInitialized() const {
   return task_queue_->ready();
 }

extensions/browser/state_store.h

@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -82,6 +83,10 @@ class StateStore : public base::SupportsWeakPtr<StateStore>,
   void AddObserver(TestObserver* observer);
   void RemoveObserver(TestObserver* observer);
 
+  // Flushes the state store (finishes any pending reads and writes). Should
+  // only be used for testing. Invokes |flushed_callback| upon completion.
+  void FlushForTesting(base::OnceClosure flushed_callback);
+
  private:
   class DelayedTaskQueue;