Fetch Finch seed during restore

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
index 17d71bd33a50c926c285c32722bd24fc02276171..1311f93eef37efd5e65d0028cfcd8f834c2219e9 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
@@ -13,12 +13,14 @@ import android.os.ParcelFileDescriptor;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
+import org.chromium.base.PathUtils;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.library_loader.ProcessInitException;
 import org.chromium.chrome.browser.firstrun.FirstRunSignInProcessor;
 import org.chromium.chrome.browser.firstrun.FirstRunStatus;
+import org.chromium.chrome.browser.init.AsyncInitTaskRunner;
 import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
 import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager;
 import org.chromium.components.signin.AccountManagerHelper;
@@ -32,6 +34,8 @@ import java.io.ObjectOutputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Backup agent for Chrome, using Android key/value backup.
@@ -52,6 +56,10 @@ public class ChromeBackupAgent extends BackupAgent {
             PrivacyPreferencesManager.PREF_METRICS_REPORTING,
     };
 
+    // Timeout for running the background tasks, needs to be quite long since they may be doing
+    // network access, but must be less than the restore timeout to be useful.

[Bernhard Bauer, 2017/01/12 15:56:02]

What is the restore timeout?

[aberent, 2017/01/12 17:31:50]

Done.

+    private static final long BACKGROUND_TASK_TIMEOUT = 20;

[Bernhard Bauer, 2017/01/12 15:56:02]

Add a unit?

[aberent, 2017/01/12 17:31:50]

Done.

+
     /**
      * Class to save and restore the backup state, used to decide if backups are needed. Since the
      * backup data is small, and stored as private data by the backup service, this can simply store
@@ -109,7 +117,7 @@ public class ChromeBackupAgent extends BackupAgent {
         try {
             ChromeBrowserInitializer.getInstance(context).handleSynchronousStartup();
         } catch (ProcessInitException e) {
-            Log.w(TAG, "Browser launch failed on restore: " + e);
+            Log.w(TAG, "Browser launch failed on backup or restore: " + e);
             return false;
         }
         return true;
@@ -225,16 +233,61 @@ public class ChromeBackupAgent extends BackupAgent {
                 backupValues.add(buffer);
             }
         }
+        // Chrome library loading depends on PathUtils, and setPrivateDataDirectorySuffix has to
+        // run on the UI thread.
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                PathUtils.setPrivateDataDirectorySuffix(
+                        ChromeBrowserInitializer.PRIVATE_DATA_DIRECTORY_SUFFIX);
+            }
+        });
 
-        // Chrome has to be running before it can check if the account exists.
+        // Start and wait for the Async init tasks. This loads the library, and attempts to load the
+        // first run variations seed. These are done asynchronously for two reasons:
+        //
+        // 1. Fetching the seed involves starting a service and waiting for a broadcast. This is
+        // received asynchronously on the UI thread. The current code doesn't support fetching the
+        // seed synchronously.
+        //
+        // 2. Performance. There are two independent long tasks to run, and Chrome can save time by
+        // running them in parallel.
+        //
+        // Note that this depends on onRestore being run from a background thread, since
+        // if it were called from the UI thread the broadcast would not be received until after it
+        // exited.
+        final CountDownLatch latch = new CountDownLatch(1);
+        new AsyncInitTaskRunner() {
+
+            @Override
+            public void onSuccess() {
+                latch.countDown();
+            }
+
+            @Override
+            public void onFailure() {
+                // Ignore failure. Problems with the variation seed can be ignored, and other
+                // problems will either recover or be repeated when Chrome is started synchronously.
+                latch.countDown();
+            }
+        }.startBackgroundTasks(false, true);

[Alexei Svitkine (slow), 2017/01/12 16:06:28]

Can you add /* */ comments documenting the params or add vars for them above so
it's clear what you're passing?

[aberent, 2017/01/12 17:31:50]

Done.

+
+        try {
+            latch.await(BACKGROUND_TASK_TIMEOUT, TimeUnit.SECONDS);
+        } catch (InterruptedException e) {
+            // Ignore timeouts. Problems with the variation seed can be ignored, and other

[Bernhard Bauer, 2017/01/12 15:56:02]

Nit: If this should time out, await() is not going to throw an
InterruptedException. That happens if something signals the current thread (via
Thread.interrupt(); see
https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html),
which to my knowledge we never do in Chrome. If it does timeout, the method is
going to return false, which we already ignore, but the comment is a bit
misleading if it's in the catch clause.

[aberent, 2017/01/12 17:31:50]

Done.

+            // problems will either recover or be repeated when Chrome is started synchronously
+        }
+        // Chrome has to be running before it can check if the account exists. Because the native
+        // library is already loaded Chrome startup should be fast.
         final ChromeBackupAgent backupAgent = this;
         if (!ThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
-                @Override
-                public Boolean call() {
-                    // Start the browser if necessary.
-                    return initializeBrowser(backupAgent);
-                }
-            })) {
+            @Override
+            public Boolean call() {
+                // Start the browser if necessary.
+                return initializeBrowser(backupAgent);
+            }
+        })) {

[Bernhard Bauer, 2017/01/12 15:56:02]

Nit: this looks really awkward. Can we maybe extract the return value if a local
variable to break up the two blocks?

[aberent, 2017/01/12 17:31:50]

Done. Also similar change in onBackup.

             // Something went wrong starting Chrome, skip the restore.
             return;
         }