Implement unlimitedStorage content capability

chrome/browser/extensions/extension_special_storage_policy.cc

Index: chrome/browser/extensions/extension_special_storage_policy.cc
diff --git a/chrome/browser/extensions/extension_special_storage_policy.cc b/chrome/browser/extensions/extension_special_storage_policy.cc
index 79d06547fd0cb242787cb8fe637ae4b7dd8a28fd..7cfbf308a3189891044bcaa6a80d1d71e172ca15 100644
--- a/chrome/browser/extensions/extension_special_storage_policy.cc
+++ b/chrome/browser/extensions/extension_special_storage_policy.cc
@@ -25,6 +25,7 @@
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
+#include "extensions/common/manifest_handlers/content_capabilities_handler.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "storage/browser/quota/quota_manager.h"
 #include "storage/common/quota/quota_status_code.h"
@@ -96,7 +97,9 @@ bool ExtensionSpecialStoragePolicy::IsStorageUnlimited(const GURL& origin) {
     return true;
 
   base::AutoLock locker(lock_);
-  return unlimited_extensions_.Contains(origin);
+  return unlimited_extensions_.Contains(origin) ||
+         content_capabilities_unlimited_extensions_.GrantsCapabilitiesTo(
+             origin);
 }
 
 bool ExtensionSpecialStoragePolicy::IsStorageSessionOnly(const GURL& origin) {
@@ -152,18 +155,21 @@ void ExtensionSpecialStoragePolicy::GrantRightsForExtension(
     const extensions::Extension* extension,
     content::BrowserContext* browser_context) {
   DCHECK(extension);
-  if (!(NeedsProtection(extension) ||
-        extension->permissions_data()->HasAPIPermission(
-            APIPermission::kUnlimitedStorage) ||
-        extension->permissions_data()->HasAPIPermission(
-            APIPermission::kFileBrowserHandler) ||
-        extensions::AppIsolationInfo::HasIsolatedStorage(extension) ||
-        extension->is_app())) {
-    return;
-  }
 
   int change_flags = 0;
-  {
+  if (extensions::ContentCapabilitiesInfo::Get(extension)
+          .permissions.count(APIPermission::kUnlimitedStorage) > 0) {
+    content_capabilities_unlimited_extensions_.Add(extension);
+    change_flags |= SpecialStoragePolicy::STORAGE_UNLIMITED;
+  }
+
+  if (NeedsProtection(extension) ||
+      extension->permissions_data()->HasAPIPermission(
+          APIPermission::kUnlimitedStorage) ||
+      extension->permissions_data()->HasAPIPermission(
+          APIPermission::kFileBrowserHandler) ||
+      extensions::AppIsolationInfo::HasIsolatedStorage(extension) ||
+      extension->is_app()) {

[not at google - send to devlin, 2014/12/12 21:03:06]

Bleh, I don't like this code. It's basically:

if (A || B || C || D || E) {
  if (A && A') ...;
  if (B) ...;
  if (C) ...;
  if (D) ...;
  if (E) ...;
}

The only reason for the highly redundant looking outer check is so that you only
need to Lock under slightly tighter conditions, but those "conditions" include
it being an app. I really don't see the point.

Which makes me realise that the change you made:

content_capabilities_unlimited_extensions_.Add(extension);

is, in theory, not thread safe. However, neither are some of the other methods
in this class (like HasSessionOnlyOrigins). How strange.

I vote:
1. Put the AutoLock(lock_) at the top of this method.
2. Delete the if (A || B || C || D || E) outer check.

[Ken Rockot(use gerrit already), 2014/12/12 21:10:17]

Doh, you're right. Moving the lock.

     base::AutoLock locker(lock_);
     if (NeedsProtection(extension) && protected_apps_.Add(extension))
       change_flags |= SpecialStoragePolicy::STORAGE_PROTECTED;
@@ -176,7 +182,6 @@ void ExtensionSpecialStoragePolicy::GrantRightsForExtension(
         unlimited_extensions_.Add(extension)) {
       if (extension->is_hosted_app())
         LogHostedAppUnlimitedStorageUsage(extension, browser_context);
-
       change_flags |= SpecialStoragePolicy::STORAGE_UNLIMITED;
     }
 
@@ -197,17 +202,21 @@ void ExtensionSpecialStoragePolicy::GrantRightsForExtension(
 void ExtensionSpecialStoragePolicy::RevokeRightsForExtension(
     const extensions::Extension* extension) {
   DCHECK(extension);
-  if (!(NeedsProtection(extension) ||
-        extension->permissions_data()->HasAPIPermission(
-            APIPermission::kUnlimitedStorage) ||
-        extension->permissions_data()->HasAPIPermission(
-            APIPermission::kFileBrowserHandler) ||
-        extensions::AppIsolationInfo::HasIsolatedStorage(extension) ||
-        extension->is_app())) {
-    return;
-  }
+
   int change_flags = 0;
-  {
+  if (extensions::ContentCapabilitiesInfo::Get(extension)
+          .permissions.count(APIPermission::kUnlimitedStorage) > 0) {
+    content_capabilities_unlimited_extensions_.Remove(extension);
+    change_flags |= SpecialStoragePolicy::STORAGE_UNLIMITED;
+  }
+
+  if (NeedsProtection(extension) ||
+      extension->permissions_data()->HasAPIPermission(
+          APIPermission::kUnlimitedStorage) ||
+      extension->permissions_data()->HasAPIPermission(
+          APIPermission::kFileBrowserHandler) ||
+      extensions::AppIsolationInfo::HasIsolatedStorage(extension) ||
+      extension->is_app()) {

[not at google - send to devlin, 2014/12/12 21:03:06]

Here too.

     base::AutoLock locker(lock_);
     if (NeedsProtection(extension) && protected_apps_.Remove(extension))
       change_flags |= SpecialStoragePolicy::STORAGE_PROTECTED;
@@ -242,6 +251,7 @@ void ExtensionSpecialStoragePolicy::RevokeRightsForAllExtensions() {
     unlimited_extensions_.Clear();
     file_handler_extensions_.Clear();
     isolated_extensions_.Clear();
+    content_capabilities_unlimited_extensions_.Clear();
   }
 
   NotifyCleared();
@@ -298,6 +308,17 @@ bool ExtensionSpecialStoragePolicy::SpecialCollection::Contains(
   return !ExtensionsContaining(origin)->is_empty();
 }
 
+bool ExtensionSpecialStoragePolicy::SpecialCollection::GrantsCapabilitiesTo(
+    const GURL& origin) {
+  for (scoped_refptr<const Extension> extension : extensions_) {

[not at google - send to devlin, 2014/12/12 21:03:06]

Should this be content_capabilities_unlimited_extensions_?

(Also same thread safety question).

[Ken Rockot(use gerrit already), 2014/12/12 21:10:17]

No, we're in a different data structure here. There is only extensions_, and
this operation is meant to be called from within a held lock scope (which it
is).

+    if (extensions::ContentCapabilitiesInfo::Get(extension.get())
+            .url_patterns.MatchesURL(origin)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 const extensions::ExtensionSet*
 ExtensionSpecialStoragePolicy::SpecialCollection::ExtensionsContaining(
     const GURL& origin) {