BitmapFetcher: Let BitmapFetcher handle data: URLs correctly

The omnibox uses BitmapFetcher to get Entity Suggestion images.

For on-focus suggestions, sometimes the images are included inline with
the Suggest results as data: URLs. This currently fails to render,
because BitmapFetcher cannot handle them.

This CL makes BitmapFetcher handle them and includes a test.

I implemented this functionality as part of BitmapFetcher instead of
further upstream, because there are multiple callers to BitmapFetcher
that may also run into this gotcha in the future, and it would be
unfortunate if they each had to write their own code to handle data:
URLs.

Bug: 1015174
Change-Id: Ibb68f0d0efdcaa8107d486dd88e3e3ccb50e2b84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2017962Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Rachel Blum <groby@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736635}

chrome/browser/bitmap_fetcher/OWNERS

-groby@chromium.org
 petewil@chromium.org
+tommycli@chromium.org
 # COMPONENT: Internals>Services>Network

chrome/browser/bitmap_fetcher/bitmap_fetcher.cc

@@ -5,12 +5,15 @@
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
 
 #include "base/bind.h"
+#include "base/task/post_task.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
+#include "net/base/data_url.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
+#include "url/url_constants.h"
 
 BitmapFetcher::BitmapFetcher(
     const GURL& url,
@@ -37,9 +40,24 @@ void BitmapFetcher::Init(const std::string& referrer,
 }
 
 void BitmapFetcher::Start(network::mojom::URLLoaderFactory* loader_factory) {
+  network::SimpleURLLoader::BodyAsStringCallback callback = base::BindOnce(
+      &BitmapFetcher::OnSimpleLoaderComplete, weak_factory_.GetWeakPtr());
+
+  // Early exit to handle data URLs.
+  if (url_.SchemeIs(url::kDataScheme)) {
+    std::string mime_type, charset, data;
+    std::unique_ptr<std::string> response_body;
+    if (net::DataURL::Parse(url_, &mime_type, &charset, &data))
+      response_body = std::make_unique<std::string>(std::move(data));
+
+    // Post a task to maintain our guarantee that the delegate will only be
+    // called asynchronously.
+    base::PostTask(FROM_HERE,
+                   BindOnce(std::move(callback), std::move(response_body)));
+    return;
+  }
+
   if (simple_loader_) {
-    network::SimpleURLLoader::BodyAsStringCallback callback = base::BindOnce(
-        &BitmapFetcher::OnSimpleLoaderComplete, base::Unretained(this));
     simple_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
         loader_factory, std::move(callback));
   }

chrome/browser/bitmap_fetcher/bitmap_fetcher.h

@@ -9,6 +9,7 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher_delegate.h"
 #include "chrome/browser/image_decoder/image_decoder.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
@@ -20,7 +21,7 @@
 
 class SkBitmap;
 
-// Asynchrounously fetches an image from the given URL and returns the
+// Asynchronously fetches an image from the given URL and returns the
 // decoded Bitmap to the provided BitmapFetcherDelegate.
 class BitmapFetcher : public ImageDecoder::ImageRequest {
  public:
@@ -31,27 +32,24 @@ class BitmapFetcher : public ImageDecoder::ImageRequest {
 
   const GURL& url() const { return url_; }
 
-  // Initializes internal fetcher.  After this function returns url_fetcher()
-  // can be accessed to configure it further (eg. add user data to request).
-  // All configuration must be done before Start() is called.
   // |credentials_mode| determines whether credentials such as cookies should be
   // sent.  Init may be called more than once in some cases.  If so, subsequent
-  // starts will be ignored.
+  // calls will be ignored.
+  // TODO(tommycli): Init and Start should likely be combined.
   virtual void Init(const std::string& referrer,
                     net::URLRequest::ReferrerPolicy referrer_policy,
                     network::mojom::CredentialsMode credentials_mode);
 
   // Start fetching the URL with the fetcher. The delegate is notified
   // asynchronously when done.  Start may be called more than once in some
-  // cases.  If so, subsequent starts will be ignored since the operation is
+  // cases.  If so, subsequent calls will be ignored since the operation is
   // already in progress.
   virtual void Start(network::mojom::URLLoaderFactory* loader_factory);
 
   // Methods inherited from ImageDecoder::ImageRequest
 
   // Called when image is decoded. |decoder| is used to identify the image in
-  // case of decoding several images simultaneously.  This will not be called
-  // on the UI thread.
+  // case of decoding several images simultaneously.
   void OnImageDecoded(const SkBitmap& decoded_image) override;
 
   // Called when decoding image failed.
@@ -69,6 +67,8 @@ class BitmapFetcher : public ImageDecoder::ImageRequest {
   BitmapFetcherDelegate* const delegate_;
   const net::NetworkTrafficAnnotationTag traffic_annotation_;
 
+  base::WeakPtrFactory<BitmapFetcher> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BitmapFetcher);
 };
 

chrome/browser/bitmap_fetcher/bitmap_fetcher_browsertest.cc

@@ -169,11 +169,7 @@ IN_PROC_BROWSER_TEST_F(BitmapFetcherBrowserTest, StartTest) {
 
 IN_PROC_BROWSER_TEST_F(BitmapFetcherBrowserTest, OnImageDecodedTest) {
   GURL url = embedded_test_server()->GetURL(kOnImageDecodedTestURL);
-  SkBitmap image;
-
-  // Put a real bitmap into "image".  2x2 bitmap of green 16 bit pixels.
-  image.allocN32Pixels(2, 2);
-  image.eraseColor(SK_ColorGREEN);
+  SkBitmap image = test_bitmap();
 
   BitmapFetcherTestDelegate delegate(kSyncCall);
 
@@ -232,3 +228,46 @@ IN_PROC_BROWSER_TEST_F(BitmapFetcherBrowserTest, HandleImageFailedTest) {
 
   EXPECT_FALSE(delegate.success());
 }
+
+IN_PROC_BROWSER_TEST_F(BitmapFetcherBrowserTest, DataURLNonImage) {
+  GURL url("data:,Hello");
+  BitmapFetcherTestDelegate delegate(kAsyncCall);
+  BitmapFetcher fetcher(url, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  fetcher.Init(
+      std::string(),
+      net::URLRequest::REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN,
+      network::mojom::CredentialsMode::kInclude);
+  fetcher.Start(
+      content::BrowserContext::GetDefaultStoragePartition(browser()->profile())
+          ->GetURLLoaderFactoryForBrowserProcess()
+          .get());
+  delegate.Wait();
+
+  EXPECT_FALSE(delegate.success());
+}
+
+IN_PROC_BROWSER_TEST_F(BitmapFetcherBrowserTest, DataURLImage) {
+  // This is test_bitmap() in data: URL form.
+  GURL url(
+      "data:image/"
+      "png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAYAAABytg0kAAAAFElEQVQYlWNk+"
+      "M/wn4GBgYGJAQoAHhgCAh6X4CYAAAAASUVORK5CYII=");
+
+  BitmapFetcherTestDelegate delegate(kAsyncCall);
+  BitmapFetcher fetcher(url, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  fetcher.Init(
+      std::string(),
+      net::URLRequest::REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN,
+      network::mojom::CredentialsMode::kInclude);
+  fetcher.Start(
+      content::BrowserContext::GetDefaultStoragePartition(browser()->profile())
+          ->GetURLLoaderFactoryForBrowserProcess()
+          .get());
+  delegate.Wait();
+
+  // Ensure image is marked as succeeded.
+  EXPECT_TRUE(delegate.success());
+  EXPECT_TRUE(gfx::BitmapsAreEqual(test_bitmap(), delegate.bitmap()));
+}