Fix blob url loading in <webview> in guest mode.

As it turns out Chrome OS guest mode is violating the assumption
<webview> made that a chrome app's storage partition is always on disk.
Because of this, before the change that added the blob url fallback
mechanism a <webview> with no explicit partition name set would
(incorrectly) share the storage partition with the app that embeds it.
Adding the blob url fallback flag fixed that (by making them separate
configs only differing in the fallback flag), but the fallback mechanism
also assumed that a chrome app's storage partition always was on disk.

This CL fixes loading blob URLs in a <webview> in guest mode by changing
StoragePartitionConfig to store a tri-state enum rather than a simple
bool for its blob url fallback mode. This way we can support fallback
regardless of if the chrome app's storage is in memory or on disk.

Bug: 1128330
Change-Id: I518cd5d1e89491eaf1cc6e2ed9ddb5cd3f0b9e87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439110Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812313}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -494,6 +494,7 @@ WRAPPED_INSTANTIATE_TEST_SUITE_P(
         TestCase("openQuickViewAudioWithImageMetadata"),
         TestCase("openQuickViewImageJpg"),
         TestCase("openQuickViewImageJpeg"),
+        TestCase("openQuickViewImageJpeg").InGuestMode(),
         TestCase("openQuickViewImageExif"),
         TestCase("openQuickViewImageRaw"),
         TestCase("openQuickViewImageRawWithOrientation"),

chrome/browser/extensions/api/identity/web_auth_flow.cc

@@ -140,7 +140,7 @@ void WebAuthFlow::DetachDelegateAndDelete() {
 
 content::StoragePartition* WebAuthFlow::GetGuestPartition() {
   return content::BrowserContext::GetStoragePartition(
-      profile_, GetWebViewPartitionConfig(partition_));
+      profile_, GetWebViewPartitionConfig(partition_, profile_));
 }
 
 const std::string& WebAuthFlow::GetAppWindowKey() const {
@@ -149,13 +149,19 @@ const std::string& WebAuthFlow::GetAppWindowKey() const {
 
 // static
 content::StoragePartitionConfig WebAuthFlow::GetWebViewPartitionConfig(
-    Partition partition) {
+    Partition partition,
+    content::BrowserContext* browser_context) {
   // This has to mirror the logic in WebViewGuest::CreateWebContents for
   // creating the correct StoragePartitionConfig.
   auto result = content::StoragePartitionConfig::Create(
       extension_misc::kIdentityApiUiAppId, GetPartitionName(partition),
       /*in_memory=*/true);
-  result.set_fallback_to_partition_domain_for_blob_urls(true);
+  result.set_fallback_to_partition_domain_for_blob_urls(
+      browser_context->IsOffTheRecord()
+          ? content::StoragePartitionConfig::FallbackMode::
+                kFallbackPartitionInMemory
+          : content::StoragePartitionConfig::FallbackMode::
+                kFallbackPartitionOnDisk);
   return result;
 }
 

chrome/browser/extensions/api/identity/web_auth_flow.h

@@ -105,7 +105,8 @@ class WebAuthFlow : public content::NotificationObserver,
   // Returns the StoragePartitionConfig for a given |partition| used in the
   // WebAuthFlow.
   static content::StoragePartitionConfig GetWebViewPartitionConfig(
-      Partition partition);
+      Partition partition,
+      content::BrowserContext* browser_context);
 
  private:
   friend class ::WebAuthFlowTest;

chrome/browser/signin/header_modification_delegate_impl.cc

@@ -109,7 +109,8 @@ bool HeaderModificationDelegateImpl::ShouldIgnoreGuestWebViewRequest(
     GURL identity_api_site =
         extensions::WebViewGuest::GetSiteForGuestPartitionConfig(
             extensions::WebAuthFlow::GetWebViewPartitionConfig(
-                extensions::WebAuthFlow::GET_AUTH_TOKEN));
+                extensions::WebAuthFlow::GET_AUTH_TOKEN,
+                contents->GetBrowserContext()));
     if (contents->GetSiteInstance()->GetSiteURL() != identity_api_site)
       return true;
 

content/public/browser/storage_partition_config.cc

@@ -51,16 +51,13 @@ StoragePartitionConfig StoragePartitionConfig::CopyWithInMemorySet() const {
 
 base::Optional<StoragePartitionConfig>
 StoragePartitionConfig::GetFallbackForBlobUrls() const {
-  if (!fallback_to_partition_domain_for_blob_urls_)
+  if (fallback_to_partition_domain_for_blob_urls_ == FallbackMode::kNone)
     return base::nullopt;
 
-  // Currently this fallback mechanism is only used in cases where the fallback
-  // partition is not in memory. If we ever need to support falling back to a
-  // in memory storage partition, we could change the fallback flag from the
-  // boolean it is today to some kind of enum.
-  // TODO(acolwell): Make this a little more robust.
-  return StoragePartitionConfig(partition_domain_, "",
-                                /*in_memory=*/false);
+  return StoragePartitionConfig(
+      partition_domain_, "",
+      /*in_memory=*/fallback_to_partition_domain_for_blob_urls_ ==
+          FallbackMode::kFallbackPartitionInMemory);
 }
 
 bool StoragePartitionConfig::operator<(

content/public/browser/storage_partition_config.h

@@ -50,18 +50,27 @@ class CONTENT_EXPORT StoragePartitionConfig {
 
   // In some cases we want a "child" storage partition to resolve blob URLs that
   // were created by their "parent", while not allowing the reverse. To enable
-  // this, set this flag to true, which will result in the storage partition
-  // with the same partition_domain but empty partition_name being used as
-  // fallback for the purpose of resolving blob URLs.
-  void set_fallback_to_partition_domain_for_blob_urls(bool fallback) {
-    if (fallback) {
+  // this, set this flag to a value other than kNone, which will result in the
+  // storage partition with the same partition_domain but empty partition_name
+  // being used as fallback for the purpose of resolving blob URLs.
+  enum class FallbackMode {
+    kNone,
+    kFallbackPartitionOnDisk,
+    kFallbackPartitionInMemory,
+  };
+  void set_fallback_to_partition_domain_for_blob_urls(FallbackMode fallback) {
+    if (fallback != FallbackMode::kNone) {
       DCHECK(!is_default());
       DCHECK(!partition_domain_.empty());
-      DCHECK(!partition_name_.empty() || in_memory_);
+      // TODO(acollwell): Ideally we shouldn't have storage partition configs
+      // that differ only in their fallback mode, but unfortunately that isn't
+      // true. When that is fixed this can be made more robust by disallowing
+      // fallback from storage partitions with an empty partition name.
+      // DCHECK(!partition_name_.empty());
     }
     fallback_to_partition_domain_for_blob_urls_ = fallback;
   }
-  bool fallback_to_partition_domain_for_blob_urls() const {
+  FallbackMode fallback_to_partition_domain_for_blob_urls() const {
     return fallback_to_partition_domain_for_blob_urls_;
   }
   base::Optional<StoragePartitionConfig> GetFallbackForBlobUrls() const;
@@ -80,7 +89,8 @@ class CONTENT_EXPORT StoragePartitionConfig {
   std::string partition_domain_;
   std::string partition_name_;
   bool in_memory_ = false;
-  bool fallback_to_partition_domain_for_blob_urls_ = false;
+  FallbackMode fallback_to_partition_domain_for_blob_urls_ =
+      FallbackMode::kNone;
 };
 
 }  // namespace content

extensions/browser/guest_view/web_view/web_view_guest.cc

@@ -90,6 +90,11 @@ namespace extensions {
 
 namespace {
 
+// Strings used to encode blob url fallback mode in site URLs.
+constexpr char kNoFallback[] = "nofallback";
+constexpr char kInMemoryFallback[] = "inmemoryfallback";
+constexpr char kOnDiskFallback[] = "ondiskfallback";
+
 // Returns storage partition removal mask from web_view clearData mask. Note
 // that storage partition mask is a subset of webview's data removal mask.
 uint32_t GetStoragePartitionRemovalMask(uint32_t web_view_removal_mask) {
@@ -296,9 +301,24 @@ bool WebViewGuest::GetGuestPartitionConfigForSite(
   // partition_domain but empty partition_name. Setting this flag on the
   // partition config causes it to be used as fallback for the purpose of
   // resolving blob URLs.
-  bool use_fallback = site.ref() != "nofallback";
+
+  // Default to having the fallback partition on disk, as that matches most
+  // closely what we would have done before fallback behavior started being
+  // encoded in the site URL.
+  content::StoragePartitionConfig::FallbackMode fallback_mode =
+      content::StoragePartitionConfig::FallbackMode::kFallbackPartitionOnDisk;
+  if (site.ref() == kNoFallback) {
+    fallback_mode = content::StoragePartitionConfig::FallbackMode::kNone;
+  } else if (site.ref() == kInMemoryFallback) {
+    fallback_mode = content::StoragePartitionConfig::FallbackMode::
+        kFallbackPartitionInMemory;
+  } else if (site.ref() == kOnDiskFallback) {
+    fallback_mode =
+        content::StoragePartitionConfig::FallbackMode::kFallbackPartitionOnDisk;
+  }
+
   storage_partition_config->set_fallback_to_partition_domain_for_blob_urls(
-      use_fallback);
+      fallback_mode);
   return true;
 }
 
@@ -307,14 +327,26 @@ GURL WebViewGuest::GetSiteForGuestPartitionConfig(
     const content::StoragePartitionConfig& storage_partition_config) {
   std::string url_encoded_partition = net::EscapeQueryParamValue(
       storage_partition_config.partition_name(), false);
-  return GURL(base::StringPrintf(
-      "%s://%s/%s?%s%s", content::kGuestScheme,
-      storage_partition_config.partition_domain().c_str(),
-      storage_partition_config.in_memory() ? "" : "persist",
-      url_encoded_partition.c_str(),
-      storage_partition_config.fallback_to_partition_domain_for_blob_urls()
-          ? ""
-          : "#nofallback"));
+  const char* fallback = "";
+  switch (
+      storage_partition_config.fallback_to_partition_domain_for_blob_urls()) {
+    case content::StoragePartitionConfig::FallbackMode::kNone:
+      fallback = kNoFallback;
+      break;
+    case content::StoragePartitionConfig::FallbackMode::
+        kFallbackPartitionOnDisk:
+      fallback = kOnDiskFallback;
+      break;
+    case content::StoragePartitionConfig::FallbackMode::
+        kFallbackPartitionInMemory:
+      fallback = kInMemoryFallback;
+      break;
+  }
+  return GURL(
+      base::StringPrintf("%s://%s/%s?%s#%s", content::kGuestScheme,
+                         storage_partition_config.partition_domain().c_str(),
+                         storage_partition_config.in_memory() ? "" : "persist",
+                         url_encoded_partition.c_str(), fallback));
 }
 
 // static
@@ -379,8 +411,16 @@ void WebViewGuest::CreateWebContents(const base::DictionaryValue& create_params,
         extensions::util::GetStoragePartitionConfigForExtensionId(
             GetOwnerSiteURL().host(),
             owner_render_process_host->GetBrowserContext());
-    if (!owner_config.is_default() && !owner_config.in_memory()) {
-      partition_config.set_fallback_to_partition_domain_for_blob_urls(true);
+    if (owner_render_process_host->GetBrowserContext()->IsOffTheRecord()) {
+      owner_config = owner_config.CopyWithInMemorySet();
+    }
+    if (!owner_config.is_default()) {
+      partition_config.set_fallback_to_partition_domain_for_blob_urls(
+          owner_config.in_memory()
+              ? content::StoragePartitionConfig::FallbackMode::
+                    kFallbackPartitionInMemory
+              : content::StoragePartitionConfig::FallbackMode::
+                    kFallbackPartitionOnDisk);
       DCHECK(owner_config == partition_config.GetFallbackForBlobUrls().value());
     }
   }