Skip to content

Commit

Permalink
android: Only call ContentMain once
Browse files Browse the repository at this point in the history 
Previously, code incorrectly assumed it's safe to call ContentMain
more than once, and thus relied on it for correctness. Synchronous
start depends on the second ContentMain call to synchronous flush
any start up tasks.

This CL fixes this by adding a direct call to flush any pending
start up tasks, and then flush instead of calling ContentMain
again.

BUG=724994

Change-Id: Ic91634ca0e317a7cf6aa0f87b23691271cd13a05
Reviewed-on: https://chromium-review.googlesource.com/550920
Commit-Queue: Bo Liu <boliu@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483163}
  • Loading branch information
Bo Liu authored and Commit Bot committed Jun 28, 2017
1 parent 5afd9fd commit 79c92c3
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 93 deletions.
13 changes: 4 additions & 9 deletions content/app/android/content_main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,10 @@ LazyInstance<std::unique_ptr<ContentMainDelegate>>::DestructorAtExit
static jint Start(JNIEnv* env, const JavaParamRef<jclass>& clazz) {
TRACE_EVENT0("startup", "content::Start");

// On Android we can have multiple requests to start the browser in process
// simultaneously. If we get an asynchonous request followed by a synchronous
// request then we have to call this a second time to finish starting the
// browser synchronously.
if (!g_service_manager_main_delegate.Get()) {
g_service_manager_main_delegate.Get() =
base::MakeUnique<ContentServiceManagerMainDelegate>(
ContentMainParams(g_content_main_delegate.Get().get()));
}
DCHECK(!g_service_manager_main_delegate.Get());
g_service_manager_main_delegate.Get() =
base::MakeUnique<ContentServiceManagerMainDelegate>(
ContentMainParams(g_content_main_delegate.Get().get()));

service_manager::MainParams main_params(
g_service_manager_main_delegate.Get().get());
Expand Down
10 changes: 5 additions & 5 deletions content/browser/android/browser_startup_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "base/android/jni_android.h"
#include "base/android/jni_string.h"
#include "content/browser/android/content_startup_flags.h"
#include "content/browser/browser_main_loop.h"
#include "ppapi/features/features.h"

#include "jni/BrowserStartupController_jni.h"
Expand All @@ -15,11 +16,6 @@ using base::android::JavaParamRef;

namespace content {

bool BrowserMayStartAsynchronously() {
JNIEnv* env = base::android::AttachCurrentThread();
return Java_BrowserStartupController_browserMayStartAsynchonously(env);
}

void BrowserStartupComplete(int result) {
JNIEnv* env = base::android::AttachCurrentThread();
Java_BrowserStartupController_browserStartupComplete(env, result);
Expand Down Expand Up @@ -65,4 +61,8 @@ static jboolean IsPluginEnabled(JNIEnv* env,
#endif
}

static void FlushStartupTasks(JNIEnv* env, const JavaParamRef<jclass>& clazz) {
BrowserMainLoop::GetInstance()->SynchronouslyFlushStartupTasks();
}

} // namespace content
1 change: 0 additions & 1 deletion content/browser/android/browser_startup_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

namespace content {

bool BrowserMayStartAsynchronously();
void BrowserStartupComplete(int result);
bool ShouldStartGpuProcessOnBrowserStartup();

Expand Down
58 changes: 27 additions & 31 deletions content/browser/browser_main_loop.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -890,52 +890,48 @@ void BrowserMainLoop::PreShutdown() {
void BrowserMainLoop::CreateStartupTasks() {
TRACE_EVENT0("startup", "BrowserMainLoop::CreateStartupTasks");

// First time through, we really want to create all the tasks
if (!startup_task_runner_.get()) {
DCHECK(!startup_task_runner_);
#if defined(OS_ANDROID)
startup_task_runner_ = base::MakeUnique<StartupTaskRunner>(
base::Bind(&BrowserStartupComplete),
base::ThreadTaskRunnerHandle::Get());
startup_task_runner_ = base::MakeUnique<StartupTaskRunner>(
base::Bind(&BrowserStartupComplete), base::ThreadTaskRunnerHandle::Get());
#else
startup_task_runner_ = base::MakeUnique<StartupTaskRunner>(
base::Callback<void(int)>(), base::ThreadTaskRunnerHandle::Get());
startup_task_runner_ = base::MakeUnique<StartupTaskRunner>(
base::Callback<void(int)>(), base::ThreadTaskRunnerHandle::Get());
#endif
StartupTask pre_create_threads =
base::Bind(&BrowserMainLoop::PreCreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(pre_create_threads);
StartupTask pre_create_threads =
base::Bind(&BrowserMainLoop::PreCreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(pre_create_threads);

StartupTask create_threads =
base::Bind(&BrowserMainLoop::CreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(create_threads);
StartupTask create_threads =
base::Bind(&BrowserMainLoop::CreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(create_threads);

StartupTask browser_thread_started = base::Bind(
&BrowserMainLoop::BrowserThreadsStarted, base::Unretained(this));
startup_task_runner_->AddTask(browser_thread_started);
StartupTask browser_thread_started = base::Bind(
&BrowserMainLoop::BrowserThreadsStarted, base::Unretained(this));
startup_task_runner_->AddTask(browser_thread_started);

StartupTask pre_main_message_loop_run = base::Bind(
&BrowserMainLoop::PreMainMessageLoopRun, base::Unretained(this));
startup_task_runner_->AddTask(pre_main_message_loop_run);
StartupTask pre_main_message_loop_run = base::Bind(
&BrowserMainLoop::PreMainMessageLoopRun, base::Unretained(this));
startup_task_runner_->AddTask(pre_main_message_loop_run);

#if defined(OS_ANDROID)
if (BrowserMayStartAsynchronously()) {
startup_task_runner_->StartRunningTasksAsync();
}
#endif
}
#if defined(OS_ANDROID)
if (!BrowserMayStartAsynchronously()) {
// A second request for asynchronous startup can be ignored, so
// StartupRunningTasksAsync is only called first time through. If, however,
// this is a request for synchronous startup then it must override any
// previous call for async startup, so we call RunAllTasksNow()
// unconditionally.
if (parameters_.ui_task) {
// Running inside browser tests, which relies on synchronous start.
startup_task_runner_->RunAllTasksNow();
} else {
startup_task_runner_->StartRunningTasksAsync();
}
#else
startup_task_runner_->RunAllTasksNow();
#endif
}

#if defined(OS_ANDROID)
void BrowserMainLoop::SynchronouslyFlushStartupTasks() {
startup_task_runner_->RunAllTasksNow();
}
#endif // OS_ANDROID

int BrowserMainLoop::CreateThreads() {
TRACE_EVENT0("startup,rail", "BrowserMainLoop::CreateThreads");

Expand Down
4 changes: 4 additions & 0 deletions content/browser/browser_main_loop.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ class CONTENT_EXPORT BrowserMainLoop {
return startup_trace_file_;
}

#if defined(OS_ANDROID)
void SynchronouslyFlushStartupTasks();
#endif // OS_ANDROID

#if !defined(OS_ANDROID)
// TODO(fsamuel): We should find an object to own HostFrameSinkManager on all
// platforms including Android. See http://crbug.com/732507.
Expand Down
41 changes: 22 additions & 19 deletions content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,12 @@ public interface StartupCallback {

private static BrowserStartupController sInstance;

private static boolean sBrowserMayStartAsynchronously;
private static boolean sShouldStartGpuProcessOnBrowserStartup;

private static void setAsynchronousStartup(boolean enable) {
sBrowserMayStartAsynchronously = enable;
}

private static void setShouldStartGpuProcessOnBrowserStartup(boolean enable) {
sShouldStartGpuProcessOnBrowserStartup = enable;
}

@VisibleForTesting
@CalledByNative
static boolean browserMayStartAsynchonously() {
return sBrowserMayStartAsynchronously;
}

@VisibleForTesting
@CalledByNative
static void browserStartupComplete(int result) {
Expand All @@ -101,6 +90,8 @@ static boolean shouldStartGpuProcessOnBrowserStartup() {
// Whether tasks that occur after resource extraction have been completed.
private boolean mPostResourceExtractionTasksCompleted;

private boolean mHasCalledContentStart;

// Whether the async startup of the browser process is complete.
private boolean mStartupDone;

Expand Down Expand Up @@ -170,15 +161,12 @@ public void startBrowserProcessesAsync(boolean startGpuProcess, final StartupCal
// flag that indicates that we have kicked off starting the browser process.
mHasStartedInitializingBrowserProcess = true;

setAsynchronousStartup(true);
setShouldStartGpuProcessOnBrowserStartup(startGpuProcess);
prepareToStartBrowserProcess(false, new Runnable() {
@Override
public void run() {
ThreadUtils.assertOnUiThread();
// Make sure to not call ContentMain.start twice, if startBrowserProcessesSync
// is called before this runs.
if (!sBrowserMayStartAsynchronously) return;
if (mHasCalledContentStart) return;
if (contentStart() > 0) {
// Failed. The callbacks may not have run, so run them.
enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
Expand Down Expand Up @@ -206,10 +194,16 @@ public void startBrowserProcessesSync(boolean singleProcess) throws ProcessInitE
prepareToStartBrowserProcess(singleProcess, null);
}

setAsynchronousStartup(false);
if (contentStart() > 0) {
// Failed. The callbacks may not have run, so run them.
enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
boolean startedSuccessfully = true;
if (!mHasCalledContentStart) {
if (contentStart() > 0) {
// Failed. The callbacks may not have run, so run them.
enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
startedSuccessfully = false;
}
}
if (startedSuccessfully) {
flushStartupTasks();
}
}

Expand All @@ -225,9 +219,16 @@ public void startBrowserProcessesSync(boolean singleProcess) throws ProcessInitE
*/
@VisibleForTesting
int contentStart() {
assert !mHasCalledContentStart;
mHasCalledContentStart = true;
return ContentMain.start();
}

@VisibleForTesting
void flushStartupTasks() {
nativeFlushStartupTasks();
}

/**
* @return Whether the browser process completed successfully.
*/
Expand Down Expand Up @@ -358,4 +359,6 @@ private static native void nativeSetCommandLineFlags(
private static native boolean nativeIsOfficialBuild();

private static native boolean nativeIsPluginEnabled();

private static native void nativeFlushStartupTasks();
}
44 changes: 16 additions & 28 deletions ...blic/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ private static class TestBrowserStartupController extends BrowserStartupControll
private int mStartupResult;
private boolean mLibraryLoadSucceeds;
private int mInitializedCounter = 0;
private boolean mStartupCompleteCalled;

@Override
void prepareToStartBrowserProcess(boolean singleProcess, Runnable completionCallback)
Expand All @@ -49,20 +50,24 @@ private TestBrowserStartupController() {
@Override
int contentStart() {
mInitializedCounter++;
if (BrowserStartupController.browserMayStartAsynchonously()) {
// Post to the UI thread to emulate what would happen in a real scenario.
ThreadUtils.postOnUiThread(new Runnable() {
@Override
public void run() {
BrowserStartupController.browserStartupComplete(mStartupResult);
}
});
} else {
BrowserStartupController.browserStartupComplete(mStartupResult);
}
// Post to the UI thread to emulate what would happen in a real scenario.
ThreadUtils.postOnUiThread(new Runnable() {
@Override
public void run() {
if (mStartupCompleteCalled) return;
BrowserStartupController.browserStartupComplete(mStartupResult);
}
});
return mStartupResult;
}

@Override
void flushStartupTasks() {
assert mInitializedCounter > 0;
if (mStartupCompleteCalled) return;
BrowserStartupController.browserStartupComplete(mStartupResult);
}

private int initializedCounter() {
return mInitializedCounter;
}
Expand Down Expand Up @@ -118,8 +123,6 @@ public void run() {
}
});

Assert.assertTrue("Asynchronous mode should have been set.",
BrowserStartupController.browserMayStartAsynchonously());
Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());

Expand Down Expand Up @@ -170,8 +173,6 @@ public void run() {
}
});

Assert.assertTrue("Asynchronous mode should have been set.",
BrowserStartupController.browserMayStartAsynchonously());
Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());

Expand Down Expand Up @@ -217,8 +218,6 @@ public void run() {
}
});

Assert.assertTrue("Asynchronous mode should have been set.",
BrowserStartupController.browserMayStartAsynchonously());
Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());

Expand Down Expand Up @@ -283,8 +282,6 @@ public void run() {
}
});

Assert.assertTrue("Asynchronous mode should have been set.",
BrowserStartupController.browserMayStartAsynchonously());
Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());

Expand Down Expand Up @@ -321,8 +318,6 @@ public void run() {
}
});

Assert.assertTrue("Asynchronous mode should have been set.",
BrowserStartupController.browserMayStartAsynchonously());
Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());

Expand Down Expand Up @@ -380,8 +375,6 @@ public void run() {
}
}
});
Assert.assertFalse("Synchronous mode should have been set",
BrowserStartupController.browserMayStartAsynchonously());

Assert.assertEquals("The browser process should have been initialized one time.", 1,
mController.initializedCounter());
Expand Down Expand Up @@ -413,8 +406,6 @@ public void run() {
}
}
});
Assert.assertFalse("Synchronous mode should have been set",
BrowserStartupController.browserMayStartAsynchonously());

Assert.assertEquals("The browser process should have been initialized twice.", 2,
mController.initializedCounter());
Expand Down Expand Up @@ -448,9 +439,6 @@ public void run() {
Assert.assertEquals("The browser process should have been initialized once.", 1,
mController.initializedCounter());

Assert.assertFalse("Synchronous mode should have been set",
BrowserStartupController.browserMayStartAsynchonously());

// Kick off the asynchronous startup request. This should just queue the callback.
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
@Override
Expand Down

0 comments on commit 79c92c3

Please sign in to comment.