Allow overlapping sync and async startup requests

content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java

Index: content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java b/content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
index 541c96ee7711c41a5a12ed398a0782f455384a8e..dc758beb12c863e75b1f150cb5867ca6ecff77e9 100644
--- a/content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
+++ b/content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
@@ -11,6 +11,7 @@ import android.test.suitebuilder.annotation.SmallTest;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.AdvancedMockContext;
 import org.chromium.content.common.ProcessInitException;
+import org.chromium.content.common.ResultCodes;
 
 public class BrowserStartupControllerTest extends InstrumentationTestCase {
 
@@ -18,41 +19,41 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
 
     private static class TestBrowserStartupController extends BrowserStartupController {
 
-        private boolean mThrowProcessInitException;
         private int mStartupResult;
-        private boolean mAlreadyInitialized = false;
+        private boolean mLibraryLoadSucceeds;
         private int mInitializedCounter = 0;
-        private boolean mCallbackWasSetup;
+
+        @Override
+        void prepareToStartBrowserProcess(int numRenderers) throws ProcessInitException {
+            if (!mLibraryLoadSucceeds) {

[nyquist, 2013/08/23 06:28:32]

Nit: Unnecessary braces

[aberent, 2013/08/23 11:40:21]

Required by Java Code Style document.

+                throw new ProcessInitException(ResultCodes.RESULT_CODE_NATIVE_LIBRARY_LOAD_FAILED);
+            }
+        }
 
         private TestBrowserStartupController(Context context) {
             super(context);
         }
 
-        @Override
-        void enableAsynchronousStartup() {
-            mCallbackWasSetup = true;
-        }
+

[nyquist, 2013/08/23 06:28:32]

Nit: Unnecessary newline.

[aberent, 2013/08/23 11:40:21]

Done.

 
         @Override
-        boolean initializeAndroidBrowserProcess() throws ProcessInitException {
+        void contentStart() {
             mInitializedCounter++;
-            if (mThrowProcessInitException) {
-                throw new ProcessInitException(4);
-            }
-            if (!mAlreadyInitialized) {
+            if(BrowserStartupController.browserMayStartAsynchonously()) {
                 // Post to the UI thread to emulate what would happen in a real scenario.
-                ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+                ThreadUtils.postOnUiThread(new Runnable() {
                     @Override
                     public void run() {
                         BrowserStartupController.browserStartupComplete(mStartupResult);
                     }
                 });
+            } else {
+                BrowserStartupController.browserStartupComplete(mStartupResult);
             }
-            return mAlreadyInitialized;
         }
 
-        private boolean hasBeenInitializedOneTime() {
-            return mInitializedCounter == 1;
+        private int initializedCounter() {
+            return mInitializedCounter;
         }
     }
 
@@ -92,6 +93,7 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     @SmallTest
     public void testSingleAsynchronousStartupRequest() {
         mController.mStartupResult = BrowserStartupController.STARTUP_SUCCESS;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback = new TestStartupCallback();
 
         // Kick off the asynchronous startup request.
@@ -102,9 +104,12 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // Asynchronous mode should have been set.

[nyquist, 2013/08/23 06:28:32]

Nit: Unnecessary comment. If the assert message is unclear, update that instead
of adding a separate comment. Both a comment and an assert message seems
unnecessary. Please apply this across all of the tests in this file.

Naturally, for some of these, the message might get too long then, and if so
keep the comment too. I don't think there should be many of that type though.

[aberent, 2013/08/23 11:40:21]

Done.

+        assertTrue("Asynchronous mode should have been set.",
+                BrowserStartupController.browserMayStartAsynchonously());
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.", 1,

[nyquist, 2013/08/23 06:28:32]

Nit: I know it is room for the 1 on the first line, but for the sake of
readability, could you put it at the start of the next line?

[aberent, 2013/08/23 11:40:21]

Done.

+                mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
@@ -118,6 +123,7 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     @SmallTest
     public void testMultipleAsynchronousStartupRequests() {
         mController.mStartupResult = BrowserStartupController.STARTUP_SUCCESS;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback1 = new TestStartupCallback();
         final TestStartupCallback callback2 = new TestStartupCallback();
         final TestStartupCallback callback3 = new TestStartupCallback();
@@ -142,9 +148,12 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // Asynchronous mode should have been set.
+        assertTrue("Asynchronous mode should have been set.",
+                BrowserStartupController.browserMayStartAsynchonously());
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.",
+                1, mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
@@ -164,6 +173,7 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     @SmallTest
     public void testConsecutiveAsynchronousStartupRequests() {
         mController.mStartupResult = BrowserStartupController.STARTUP_SUCCESS;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback1 = new TestStartupCallback();
         final TestStartupCallback callback2 = new TestStartupCallback();
 
@@ -181,9 +191,12 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // Asynchronous mode should have been set.
+        assertTrue("Asynchronous mode should have been set.",
+                BrowserStartupController.browserMayStartAsynchonously());
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.",
+                1, mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
@@ -226,6 +239,7 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     @SmallTest
     public void testSingleFailedAsynchronousStartupRequest() {
         mController.mStartupResult = BrowserStartupController.STARTUP_FAILURE;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback = new TestStartupCallback();
 
         // Kick off the asynchronous startup request.
@@ -236,9 +250,12 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // Asynchronous mode should have been set.
+        assertTrue("Asynchronous mode should have been set.",
+                BrowserStartupController.browserMayStartAsynchonously());
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.",
+                1, mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
@@ -250,6 +267,7 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     @SmallTest
     public void testConsecutiveFailedAsynchronousStartupRequests() {
         mController.mStartupResult = BrowserStartupController.STARTUP_FAILURE;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback1 = new TestStartupCallback();
         final TestStartupCallback callback2 = new TestStartupCallback();
 
@@ -267,9 +285,12 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // Asynchronous mode should have been set.
+        assertTrue("Asynchronous mode should have been set.",
+                BrowserStartupController.browserMayStartAsynchonously());
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.",
+                1, mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
@@ -306,32 +327,61 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
     }
 
     @SmallTest
-    public void testAndroidBrowserStartupThrowsException() {
-        mController.mThrowProcessInitException = true;
+    public void testSingleSynchronousRequest() {
+        mController.mStartupResult = BrowserStartupController.STARTUP_SUCCESS;
+        mController.mLibraryLoadSucceeds = true;
+        // Kick off the synchronous startup.
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                assertTrue("Browser should have started succesfully",

[nyquist, 2013/08/23 06:28:32]

Nit: successfully

[aberent, 2013/08/23 11:40:21]

Done.

+                        mController.startBrowserProcessesSync(1));
+            }
+        });
+        // We should have forced synchronous startup
+        assertFalse("Synchronous mode should have been set",
+                BrowserStartupController.browserMayStartAsynchonously());
+
+        // Ensure that it ends up trying to initialize the browser process.
+        assertEquals("The browser process should have been initialized one time.",
+                1, mController.initializedCounter());
+    }
+    @SmallTest

[nyquist, 2013/08/23 06:28:32]

Nit: Missing newline

[aberent, 2013/08/23 11:40:21]

Done.

+    public void testAsyncThenSyncRequests() {
+        mController.mStartupResult = BrowserStartupController.STARTUP_SUCCESS;
+        mController.mLibraryLoadSucceeds = true;
         final TestStartupCallback callback = new TestStartupCallback();
 
-        // Kick off the asynchronous startup request.
+        // Kick off the startups.
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
                 mController.startBrowserProcessesAsync(callback);
+                // To ensure that the async startup doesn't complete too soon we have
+                // to do both these in a since Runnable instance. This avoids the
+                // unpredictable race that happens in real situations.
+                assertTrue("Browser should have started succesfully",
+                        mController.startBrowserProcessesSync(1));
             }
         });
+        // We should have forced synchronous startup
+        assertFalse("Synchronous mode should have been set",
+                BrowserStartupController.browserMayStartAsynchonously());
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
-
-        // Wait for callbacks to complete.
-        getInstrumentation().waitForIdleSync();
+        // The browser process should have been called twice!
+        assertEquals("The browser process should have been initialized twice.",
+                2, mController.initializedCounter());
 
+        // Sync startup should have forced execution of our callbacks.
         assertTrue("Callback should have been executed.", callback.mHasStartupResult);
-        assertTrue("Callback should have been a failure.", callback.mWasFailure);
+        assertTrue("Callback should have been a success.", callback.mWasSuccess);
+        assertFalse("Callback should be told that the browser process was not already started.",
+                callback.mAlreadyStarted);

[nyquist, 2013/08/23 06:28:32]

Question: How do you think about adding a simple extension of this test, that
ensures that following async requests also work as expected after the
synchronous startup, to ensure that all state is correct after this?

[aberent, 2013/08/23 11:40:21]

Done. Probably a sensible extra case.

     }
 
     @SmallTest
-    public void testAndroidBrowserProcessAlreadyInitializedByOtherPartsOfCode() {
-        mController.mAlreadyInitialized = true;
+    public void testLibraryLoadFails() {
+        mController.mLibraryLoadSucceeds = false;
         final TestStartupCallback callback = new TestStartupCallback();
 
         // Kick off the asynchronous startup request.
@@ -342,16 +392,17 @@ public class BrowserStartupControllerTest extends InstrumentationTestCase {
             }
         });
 
-        assertTrue("The callback should have been setup", mController.mCallbackWasSetup);
-        assertTrue("The browser process should have been initialized one time.",
-                mController.hasBeenInitializedOneTime());
+        // The browser should not have been initialized.
+        assertEquals("The browser process should not have been initialized.",
+                0, mController.initializedCounter());
 
         // Wait for callbacks to complete.
         getInstrumentation().waitForIdleSync();
 
         assertTrue("Callback should have been executed.", callback.mHasStartupResult);
-        assertTrue("Callback should have been a success.", callback.mWasSuccess);
-        assertTrue("Callback should be told that the browser process was already started.",
+        assertFalse("Callback should have been a failure.", callback.mWasSuccess);
+        assertFalse("Callback should be told that the browser process was not already started.",
                 callback.mAlreadyStarted);
     }
+
 }