ExtensionStorageMonitor: use smaller, self-registering StorageObservers

chrome/browser/extensions/extension_storage_monitor_browsertest.cc

Index: chrome/browser/extensions/extension_storage_monitor_browsertest.cc
diff --git a/chrome/browser/extensions/extension_storage_monitor_browsertest.cc b/chrome/browser/extensions/extension_storage_monitor_browsertest.cc
index b9671a8945d6d1e405271ba4c42003ae989915d1..0dd170ddff23062b3488ff3c38f12b1026397583 100644
--- a/chrome/browser/extensions/extension_storage_monitor_browsertest.cc
+++ b/chrome/browser/extensions/extension_storage_monitor_browsertest.cc
@@ -5,14 +5,18 @@
 #include <stdint.h>
 
 #include <set>
+#include <string>
+#include <vector>
 
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_storage_monitor.h"
+#include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_dialog_auto_confirm.h"
 #include "extensions/browser/extension_prefs.h"
@@ -20,7 +24,9 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/value_builder.h"
 #include "extensions/test/extension_test_message_listener.h"
+#include "net/dns/mock_host_resolver.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/message_center_observer.h"
 
@@ -86,6 +92,8 @@ class ExtensionStorageMonitorTest : public ExtensionBrowserTest {
   void SetUpOnMainThread() override {
     ExtensionBrowserTest::SetUpOnMainThread();
 
+    host_resolver()->AddRule("*", "127.0.0.1");
+
     InitStorageMonitor();
   }
 
@@ -111,6 +119,40 @@ class ExtensionStorageMonitorTest : public ExtensionBrowserTest {
     return extension;
   }
 
+  const Extension* CreateHostedApp(const std::string& name,
+                                   GURL app_url,
+                                   std::vector<std::string> permissions) {
+    std::unique_ptr<TestExtensionDir> dir(new TestExtensionDir);

[Devlin, 2017/06/12 20:15:05]

prefer auto dir = base::MakeUnique<TestExtensionDir>();

[ncarter (slow), 2017/06/12 22:52:07]

Done.

+
+    url::Replacements<char> clear_port;
+    clear_port.ClearPort();
+
+    DictionaryBuilder manifest;
+    manifest.Set("name", name)
+        .Set("version", "1.0")
+        .Set("manifest_version", 2)
+        .Set(
+            "app",
+            DictionaryBuilder()
+                .Set("urls",
+                     ListBuilder()
+                         .Append(app_url.ReplaceComponents(clear_port).spec())
+                         .Build())
+                .Set("launch",
+                     DictionaryBuilder().Set("web_url", app_url.spec()).Build())
+                .Build());
+    ListBuilder permissions_builder;
+    for (const std::string& permission : permissions)
+      permissions_builder.Append(permission);
+    manifest.Set("permissions", permissions_builder.Build());
+    dir->WriteManifest(manifest.ToJSON());
+
+    const Extension* extension = LoadExtension(dir->UnpackedPath());
+    EXPECT_TRUE(extension);
+    temp_dirs_.push_back(std::move(dir));
+    return extension;
+  }
+
   std::string GetNotificationId(const std::string& extension_id) {
     return monitor()->GetNotificationId(extension_id);
   }
@@ -124,17 +166,22 @@ class ExtensionStorageMonitorTest : public ExtensionBrowserTest {
   }
 
   void WriteBytesExpectingNotification(const Extension* extension,
-                                       int num_bytes) {
+                                       int num_bytes,
+                                       const char* filesystem = "PERSISTENT") {
     int64_t previous_threshold = GetNextStorageThreshold(extension->id());
-    WriteBytes(extension, num_bytes, true);
-    EXPECT_GT(GetNextStorageThreshold(extension->id()), previous_threshold);
+    WriteBytes(extension, num_bytes, filesystem, true);
+    ASSERT_GT(GetNextStorageThreshold(extension->id()), previous_threshold);
   }
 
-  void WriteBytesNotExpectingNotification(const Extension* extension,
-                                         int num_bytes) {
-    WriteBytes(extension, num_bytes, false);
+  void WriteBytesNotExpectingNotification(
+      const Extension* extension,
+      int num_bytes,
+      const char* filesystem = "PERSISTENT") {
+    WriteBytes(extension, num_bytes, filesystem, false);
   }
 
+  void SimulateProfileShutdown() { storage_monitor_->StopMonitoringAll(); }
+
  private:
   void InitStorageMonitor() {
     storage_monitor_ = ExtensionStorageMonitor::Get(profile());
@@ -149,34 +196,73 @@ class ExtensionStorageMonitorTest : public ExtensionBrowserTest {
     storage_monitor_->observer_rate_ = base::TimeDelta();
   }
 
-  // Write a number of bytes to persistent storage.
-  void WriteBytes(const Extension* extension,
-                  int num_bytes,
-                  bool expected_notification) {
+  // Write bytes for a hosted app page that's loaded the script:
+  //     //chrome/test/data/extensions/storage_monitor/hosted_apps/common.js
+  void WriteBytesForHostedApp(const Extension* extension,
+                              int num_bytes,
+                              const std::string& filesystem) {
+    content::WebContents* web_contents = OpenApplication(AppLaunchParams(
+        profile(), extension, LAUNCH_CONTAINER_TAB,
+        WindowOpenDisposition::SINGLETON_TAB, extensions::SOURCE_TEST));
+
+    ASSERT_TRUE(WaitForLoadStop(web_contents));
+    std::string result;
+    ASSERT_TRUE(ExecuteScriptAndExtractString(
+        web_contents,
+        base::StringPrintf(R"(
+            HostedAppWriteData(%s, %d)
+               .then(() => domAutomationController.send('write_done'))
+               .catch(e => domAutomationController.send('write_error: ' + e));
+            )",
+                           filesystem.c_str(), num_bytes),

[Devlin, 2017/06/12 20:15:05]

... this formatting's a bit odd.  Was this git cl format?  If so, should we file
a bug, or is this desired?

[ncarter (slow), 2017/06/12 22:52:07]

Fixed by extracting the literal to a local variable. I'm still in search of a
great way to use raw strings w/ ExecuteScript, so let me know your ideas here. 

clang-format isn't allowed to mess with the whitespace inside the literal, and
it seems kind of odd to ask clang-format to align to whitespace _inside_ of a
string literal. So it's not clear to me what what bug we'd file.

+        &result));
+
+    ASSERT_EQ("write_done", result);
+  }
+
+  // Write bytes for the extension loaded from:
+  //     //chrome/test/data/extensions/storage_monitor/write_data
+  void WriteBytesForExtension(const Extension* extension, int num_bytes) {
     ExtensionTestMessageListener launched_listener("launched", true);
-    ExtensionTestMessageListener write_complete_listener(
-        "write_complete", false);
-    NotificationObserver notification_observer(
-        GetNotificationId(extension->id()));
+    ExtensionTestMessageListener write_complete_listener("write_complete",
+                                                         false);
 
     OpenApplication(AppLaunchParams(profile(), extension, LAUNCH_CONTAINER_NONE,
                                     WindowOpenDisposition::NEW_WINDOW,
                                     extensions::SOURCE_TEST));
+
     ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
 
     // Instruct the app to write |num_bytes| of data.
     launched_listener.Reply(base::IntToString(num_bytes));
     ASSERT_TRUE(write_complete_listener.WaitUntilSatisfied());
+  }
+
+  // Write a number of bytes to persistent storage.
+  void WriteBytes(const Extension* extension,
+                  int num_bytes,
+                  const std::string& filesystem,
+                  bool expected_notification) {
+    NotificationObserver notification_observer(
+        GetNotificationId(extension->id()));
+
+    if (extension->is_hosted_app()) {
+      WriteBytesForHostedApp(extension, num_bytes, filesystem);
+    } else {
+      ASSERT_EQ("PERSISTENT", filesystem) << "Not implemented in the js code.";
+      WriteBytesForExtension(extension, num_bytes);
+    }
 
     if (expected_notification) {
-      EXPECT_TRUE(notification_observer.WaitForNotification());
+      ASSERT_TRUE(notification_observer.WaitForNotification());
     } else {
       base::RunLoop().RunUntilIdle();
-      EXPECT_FALSE(notification_observer.HasReceivedNotification());
+      ASSERT_FALSE(notification_observer.HasReceivedNotification());
     }
   }
 
   ExtensionStorageMonitor* storage_monitor_;
+  std::vector<std::unique_ptr<TestExtensionDir>> temp_dirs_;
 };
 
 // Control - No notifications should be shown if usage remains under the
@@ -253,6 +339,67 @@ IN_PROC_BROWSER_TEST_F(ExtensionStorageMonitorTest,
   WriteBytesNotExpectingNotification(extension, GetInitialExtensionThreshold());
 }
 
+// Regression test for https://crbug.com/716426
+IN_PROC_BROWSER_TEST_F(ExtensionStorageMonitorTest,
+                       HostedAppTemporaryFilesystem) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url = embedded_test_server()->GetURL(
+      "chromium.org", "/extensions/storage_monitor/hosted_apps/one/index.html");
+  const Extension* app =
+      CreateHostedApp("Hosted App", url, {"unlimitedStorage"});
+
+  EXPECT_NO_FATAL_FAILURE(WriteBytesExpectingNotification(
+      app, GetInitialExtensionThreshold(), "TEMPORARY"));
+  EXPECT_NO_FATAL_FAILURE(WriteBytesNotExpectingNotification(
+      app, GetInitialExtensionThreshold(), "PERSISTENT"));
+  EXPECT_NO_FATAL_FAILURE(WriteBytesExpectingNotification(
+      app, GetInitialExtensionThreshold(), "TEMPORARY"));
+
+  // Bug 716426 was a shutdown crash due to not removing a
+  // storage::StorageObserver registration before deleting the observer. To
+  // recreate that scenario, first disable the app (which leaks the observer
+  // registration), then simulate the step of profile exit where we delete the
+  // StorageObserver.
+  DisableExtension(app->id());
+  SimulateProfileShutdown();
+
+  // Now generate more storage activity for the hosted app's temporary
+  // filesystem. Note that it's not a hosted app anymore -- it's just a webpage.
+  // Bug 716426 caused this to crash the browser.
+  EXPECT_NO_FATAL_FAILURE(WriteBytesNotExpectingNotification(
+      app, GetInitialExtensionThreshold(), "TEMPORARY"));
+}
+
+// Exercises the case where two hosted apps are same-origin but have non-
+// overlapping extents. Disabling one should not suppress storage monitoring for
+// the other.
+IN_PROC_BROWSER_TEST_F(ExtensionStorageMonitorTest, TwoHostedAppsInSameOrigin) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url1 = embedded_test_server()->GetURL(
+      "chromium.org", "/extensions/storage_monitor/hosted_apps/one/index.html");
+  const Extension* app1 = CreateHostedApp("App 1", url1, {"unlimitedStorage"});
+
+  GURL url2 = embedded_test_server()->GetURL(
+      "chromium.org", "/extensions/storage_monitor/hosted_apps/two/index.html");
+  const Extension* app2 = CreateHostedApp("App 2", url2, {"unlimitedStorage"});
+
+  EXPECT_EQ(url1.GetOrigin(), url2.GetOrigin());
+
+  EXPECT_NO_FATAL_FAILURE(
+      WriteBytesExpectingNotification(app1, GetInitialExtensionThreshold()));
+  EXPECT_NO_FATAL_FAILURE(WriteBytesExpectingNotification(
+      app2, GetInitialExtensionThreshold() * 2));
+
+  // Disable app2. We should still be monitoring the origin on behalf of app1.
+  DisableExtension(app2->id());
+
+  // Writing a bunch of data in app1 should trigger the warning.
+  EXPECT_NO_FATAL_FAILURE(WriteBytesExpectingNotification(
+      app1, GetInitialExtensionThreshold() * 4));
+}
+
 // Verify that notifications are disabled when the user clicks the action button
 // in the notification.
 // Flaky: https://crbug.com/617801