Fetch splash image earlier during web app installs.

This changes ShortcutHelper to download the splash image for a webapp
while the webapp is being installed, rather than after. If the splash
image is downloaded before the webapp installation process finishes,
the splash image will be stored in a local map until it can be
transferred to the WebappDataStorage object.

This simplifies the code and fixes a potential UAF.

Bug: 1147913
Change-Id: I9405ee9cc304eaf8b9ff2bbaca17c1c63cfd6e65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533031
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826837}

chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java

@@ -36,7 +36,6 @@ import org.chromium.base.Log;
 import org.chromium.base.StrictModeContext;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.annotations.CalledByNative;
-import org.chromium.base.annotations.NativeMethods;
 import org.chromium.base.task.AsyncTask;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.browserservices.BrowserServicesIntentDataProvider;
@@ -55,8 +54,10 @@ import org.chromium.ui.base.ViewUtils;
 import org.chromium.ui.widget.Toast;
 
 import java.io.ByteArrayOutputStream;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -147,6 +148,12 @@ public class ShortcutHelper {
 
     private static ShortcutManager sShortcutManager;
 
+    // Holds splash images for web apps that are currently being installed. After installation is
+    // complete, the image associated with the web app will be moved to the appropriate {@link
+    // WebappDataStorage}.
+    @VisibleForTesting
+    public static Map<String, Bitmap> sSplashImageMap = new HashMap<>();
+
     /** Helper for generating home screen shortcuts. */
     public static class Delegate {
         /**
@@ -195,8 +202,7 @@ public class ShortcutHelper {
 
     /**
      * Adds home screen shortcut which opens in a {@link WebappActivity}. Creates web app
-     * home screen shortcut and registers web app asynchronously. Calls
-     * ShortcutHelper::OnWebappDataStored() when done.
+     * home screen shortcut and registers web app asynchronously.
      */
     @SuppressWarnings("unused")
     @CalledByNative
@@ -204,7 +210,7 @@ public class ShortcutHelper {
             final String userTitle, final String name, final String shortName, final String iconUrl,
             final Bitmap icon, boolean isIconAdaptive, @WebDisplayMode final int displayMode,
             final int orientation, final int source, final long themeColor,
-            final long backgroundColor, final long callbackPointer) {
+            final long backgroundColor) {
         new AsyncTask<Intent>() {
             @Override
             protected Intent doInBackground() {
@@ -225,18 +231,20 @@ public class ShortcutHelper {
             protected void onPostExecute(final Intent resultIntent) {
                 sDelegate.addShortcutToHomescreen(userTitle, icon, isIconAdaptive, resultIntent);
 
-                // Store the webapp data so that it is accessible without the intent. Once this
-                // process is complete, call back to native code to start the splash image
-                // download.
+                // Store the webapp data so that it is accessible without the intent.
                 WebappRegistry.getInstance().register(id, storage -> {
                     BrowserServicesIntentDataProvider intentDataProvider =
                             WebappIntentDataProviderFactory.create(resultIntent);
                     assert intentDataProvider != null;
                     if (intentDataProvider != null) {
                         storage.updateFromWebappIntentDataProvider(intentDataProvider);
-                        if (callbackPointer != 0) {
-                            ShortcutHelperJni.get().onWebappDataStored(callbackPointer);
-                        }
+                    }
+
+                    // If the image is not yet downloaded (i.e. |splashImage| is null), it will be
+                    // stored later when native calls storeWebappSplashImage().
+                    Bitmap splashImage = sSplashImageMap.remove(id);
+                    if (splashImage != null) {
+                        storeWebappSplashImage(id, splashImage);
                     }
                 });
                 if (shouldShowToastWhenAddingShortcut()) {
@@ -329,7 +337,10 @@ public class ShortcutHelper {
     @CalledByNative
     private static void storeWebappSplashImage(final String id, final Bitmap splashImage) {
         final WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(id);
-        if (storage != null) {
+        if (storage == null) {
+            // The app is not installed yet; put it in this map for now.
+            sSplashImageMap.put(id, splashImage);
+        } else {
             new AsyncTask<String>() {
                 @Override
                 protected String doInBackground() {
@@ -812,9 +823,4 @@ public class ShortcutHelper {
             storage.setShouldForceUpdate(true);
         }
     }
-
-    @NativeMethods
-    interface Natives {
-        void onWebappDataStored(long callbackPointer);
-    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/webapps/addtohomescreen/AddToHomescreenTest.java

@@ -311,6 +311,8 @@ public class AddToHomescreenTest {
             Criteria.checkThat(dataStorageFactory.mSplashImage, Matchers.notNullValue());
         });
 
+        Assert.assertTrue(ShortcutHelper.sSplashImageMap.isEmpty());
+
         // Test that bitmap sizes match expectations.
         int idealSize = mActivity.getResources().getDimensionPixelSize(
                 R.dimen.webapp_splash_image_size_ideal);

chrome/browser/android/shortcut_helper.cc

@@ -74,15 +74,11 @@ void GetIconSizes() {
 }
 
 // Adds a shortcut which opens in a fullscreen window to the launcher.
-// |splash_image_callback| will be invoked once the Java-side operation has
-// completed. This is necessary as Java will asynchronously create and
-// populate a WebappDataStorage object for standalone-capable sites. This must
-// exist before the splash image can be stored.
-void AddWebappWithSkBitmap(const ShortcutInfo& info,
+void AddWebappWithSkBitmap(content::WebContents* web_contents,
+                           const ShortcutInfo& info,
                            const std::string& webapp_id,
                            const SkBitmap& icon_bitmap,
-                           bool is_icon_maskable,
-                           base::OnceClosure splash_image_callback) {
+                           bool is_icon_maskable) {
   // Send the data to the Java side to create the shortcut.
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jstring> java_webapp_id =
@@ -104,25 +100,23 @@ void AddWebappWithSkBitmap(const ShortcutInfo& info,
   if (!icon_bitmap.drawsNothing())
     java_bitmap = gfx::ConvertToJavaBitmap(icon_bitmap);
 
-  // The callback will need to be run after shortcut creation completes in order
-  // to download the splash image and save it to the WebappDataStorage. Create a
-  // copy of the callback here and send the pointer to Java, which will send it
-  // back once the asynchronous shortcut creation process finishes.
-  uintptr_t callback_pointer = reinterpret_cast<uintptr_t>(
-      new base::OnceClosure(std::move(splash_image_callback)));
-
   Java_ShortcutHelper_addWebapp(
       env, java_webapp_id, java_url, java_scope_url, java_user_title, java_name,
       java_short_name, java_best_primary_icon_url, java_bitmap,
       is_icon_maskable, static_cast<int>(info.display),
       static_cast<int>(info.orientation), info.source,
       OptionalSkColorToJavaColor(info.theme_color),
-      OptionalSkColorToJavaColor(info.background_color), callback_pointer);
+      OptionalSkColorToJavaColor(info.background_color));
+
+  // Start downloading the splash image in parallel with the app install.
+  content::ManifestIconDownloader::Download(
+      web_contents, info.splash_image_url, info.ideal_splash_image_size_in_px,
+      info.minimum_splash_image_size_in_px,
+      base::BindOnce(&ShortcutHelper::StoreWebappSplashImage, webapp_id));
 }
 
 // Adds a shortcut which opens in a browser tab to the launcher.
-void AddShortcutWithSkBitmap(content::WebContents* web_contents,
-                             const ShortcutInfo& info,
+void AddShortcutWithSkBitmap(const ShortcutInfo& info,
                              const std::string& id,
                              const SkBitmap& icon_bitmap,
                              bool is_icon_maskable) {
@@ -180,16 +174,11 @@ void ShortcutHelper::AddToLauncherWithSkBitmap(
   if (info.display == blink::mojom::DisplayMode::kStandalone ||
       info.display == blink::mojom::DisplayMode::kFullscreen ||
       info.display == blink::mojom::DisplayMode::kMinimalUi) {
-    AddWebappWithSkBitmap(
-        info, webapp_id, icon_bitmap, is_icon_maskable,
-        base::BindOnce(&ShortcutHelper::FetchSplashScreenImage, web_contents,
-                       info.splash_image_url,
-                       info.ideal_splash_image_size_in_px,
-                       info.minimum_splash_image_size_in_px, webapp_id));
+    AddWebappWithSkBitmap(web_contents, info, webapp_id, icon_bitmap,
+                          is_icon_maskable);
     return;
   }
-  AddShortcutWithSkBitmap(web_contents, info, webapp_id, icon_bitmap,
-                          is_icon_maskable);
+  AddShortcutWithSkBitmap(info, webapp_id, icon_bitmap, is_icon_maskable);
 }
 
 void ShortcutHelper::ShowWebApkInstallInProgressToast() {
@@ -233,21 +222,6 @@ int ShortcutHelper::GetIdealShortcutIconSizeInPx() {
   return g_ideal_shortcut_icon_size;
 }
 
-// static
-void ShortcutHelper::FetchSplashScreenImage(
-    content::WebContents* web_contents,
-    const GURL& image_url,
-    const int ideal_splash_image_size_in_px,
-    const int minimum_splash_image_size_in_px,
-    const std::string& webapp_id) {
-  // This is a fire and forget task. It is not vital for the splash screen image
-  // to be downloaded so if the downloader returns false there is no fallback.
-  content::ManifestIconDownloader::Download(
-      web_contents, image_url, ideal_splash_image_size_in_px,
-      minimum_splash_image_size_in_px,
-      base::BindOnce(&ShortcutHelper::StoreWebappSplashImage, webapp_id));
-}
-
 // static
 void ShortcutHelper::StoreWebappSplashImage(const std::string& webapp_id,
                                             const SkBitmap& splash_image) {
@@ -382,19 +356,3 @@ bool ShortcutHelper::DoesAndroidSupportMaskableIcons() {
 void ShortcutHelper::SetIdealShortcutSizeForTesting(int size) {
   g_ideal_shortcut_icon_size = size;
 }
-
-// Callback used by Java when the shortcut has been created.
-// |splash_image_callback| is a pointer to a base::OnceClosure allocated in
-// AddShortcutWithSkBitmap, so reinterpret_cast it back and run it.
-//
-// This callback should only ever be called when the shortcut was for a
-// webapp-capable site; otherwise, |splash_image_callback| will have never been
-// allocated and doesn't need to be run or deleted.
-void JNI_ShortcutHelper_OnWebappDataStored(JNIEnv* env,
-                                           jlong jsplash_image_callback) {
-  DCHECK(jsplash_image_callback);
-  base::OnceClosure* splash_image_callback =
-      reinterpret_cast<base::OnceClosure*>(jsplash_image_callback);
-  std::move(*splash_image_callback).Run();
-  delete splash_image_callback;
-}

chrome/browser/android/shortcut_helper.h

@@ -66,16 +66,6 @@ class ShortcutHelper {
   // Returns the ideal size for a shortcut icon of a WebAPK.
   static int GetIdealShortcutIconSizeInPx();
 
-  // Fetches the splash screen image and stores it inside the WebappDataStorage
-  // of the webapp. The WebappDataStorage object *must* have been previously
-  // created by AddToLauncherWithSkBitmap(); this method should be passed as a
-  // closure to that method.
-  static void FetchSplashScreenImage(content::WebContents* web_contents,
-                                     const GURL& image_url,
-                                     const int ideal_splash_image_size_in_px,
-                                     const int minimum_splash_image_size_in_px,
-                                     const std::string& webapp_id);
-
   // Stores the webapp splash screen in the WebappDataStorage associated with
   // |webapp_id|.
   static void StoreWebappSplashImage(const std::string& webapp_id,