Skip to content

Commit

Permalink
android: assert runningOnLauncherThread
Browse files Browse the repository at this point in the history 
This change is meant to have no production impact. Add assert to methods
that already only running on launcher thread in production. This involves
a lot of changes in tests though since test often assume things are
thread safe. This includes moving some helper methods from
ChildProcessLauncherTest to ChildProcessLauncherTestHelperService so
they can be shared.

Also take this opportunity to move a bunch of ForTesting methods from
ChildProcessLauncher to actual test file. This does involve exposing
other things and marking them as @VisibleForTesting, but imo removing
stuff from ChildProcessLauncher is a good thing.

BUG=689758

Review-Url: https://codereview.chromium.org/2809293005
Cr-Commit-Position: refs/heads/master@{#464498}
  • Loading branch information
boliu authored and Commit bot committed Apr 13, 2017
1 parent 092d349 commit 06114d7
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 133 deletions.
96 changes: 12 additions & 84 deletions content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public interface LaunchCallback { void onChildProcessStarted(int pid); }

private static final boolean SPARE_CONNECTION_ALWAYS_IN_FOREGROUND = false;

private static ChildProcessConnection allocateConnection(
@VisibleForTesting
static ChildProcessConnection allocateConnection(
ChildSpawnData spawnData, Bundle childProcessCommonParams, boolean forWarmUp) {
ChildProcessConnection.DeathCallback deathCallback =
new ChildProcessConnection.DeathCallback() {
Expand Down Expand Up @@ -92,7 +93,8 @@ private static ChromiumLinkerParams getLinkerParamsForNewConnection() {
}
}

private static Bundle createCommonParamsBundle(ChildProcessCreationParams params) {
@VisibleForTesting
static Bundle createCommonParamsBundle(ChildProcessCreationParams params) {
Bundle commonParams = new Bundle();
commonParams.putParcelable(
ChildProcessConstants.EXTRA_LINKER_PARAMS, getLinkerParamsForNewConnection());
Expand All @@ -101,8 +103,10 @@ private static Bundle createCommonParamsBundle(ChildProcessCreationParams params
return commonParams;
}

private static ChildProcessConnection allocateBoundConnection(ChildSpawnData spawnData,
@VisibleForTesting
static ChildProcessConnection allocateBoundConnection(ChildSpawnData spawnData,
ChildProcessConnection.StartCallback startCallback, boolean forWarmUp) {
assert LauncherThread.runningOnLauncherThread();
final Context context = spawnData.getContext();
final boolean inSandbox = spawnData.isInSandbox();
final ChildProcessCreationParams creationParams = spawnData.getCreationParams();
Expand Down Expand Up @@ -313,9 +317,9 @@ public void onChildStartFailed() {
* @param commandLine The child process command line argv.
* @param filesToBeMapped File IDs, FDs, offsets, and lengths to pass through.
*/
// TODO(boliu): All tests should use this over startForTesting.
static void start(Context context, int paramId, final String[] commandLine, int childProcessId,
FileDescriptorInfo[] filesToBeMapped, LaunchCallback launchCallback) {
assert LauncherThread.runningOnLauncherThread();
IBinder childProcessCallback = null;
boolean inSandbox = true;
boolean alwaysInForeground = false;
Expand Down Expand Up @@ -355,11 +359,13 @@ static void start(Context context, int paramId, final String[] commandLine, int
childProcessCallback, inSandbox, alwaysInForeground, params);
}

private static ChildProcessConnection startInternal(final Context context,
@VisibleForTesting
public static ChildProcessConnection startInternal(final Context context,
final String[] commandLine, final int childProcessId,
final FileDescriptorInfo[] filesToBeMapped, final LaunchCallback launchCallback,
final IBinder childProcessCallback, final boolean inSandbox,
final boolean alwaysInForeground, final ChildProcessCreationParams creationParams) {
assert LauncherThread.runningOnLauncherThread();
try {
TraceEvent.begin("ChildProcessLauncher.startInternal");

Expand Down Expand Up @@ -450,6 +456,7 @@ protected static Bundle createsServiceBundle(
static void triggerConnectionSetup(final ChildProcessConnection connection,
String[] commandLine, int childProcessId, FileDescriptorInfo[] filesToBeMapped,
final IBinder childProcessCallback, final LaunchCallback launchCallback) {
assert LauncherThread.runningOnLauncherThread();
ChildProcessConnection.ConnectionCallback connectionCallback =
new ChildProcessConnection.ConnectionCallback() {
@Override
Expand Down Expand Up @@ -489,91 +496,12 @@ static void stop(int pid) {
freeConnection(connection);
}

@VisibleForTesting
public static ChildProcessConnection startForTesting(Context context, String[] commandLine,
FileDescriptorInfo[] filesToMap, ChildProcessCreationParams params) {
return startInternal(context, commandLine, 0 /* childProcessId */, filesToMap,
null /* launchCallback */, null /* childProcessCallback */, true /* inSandbox */,
false /* alwaysInForeground */, params);
}

@VisibleForTesting
static ChildProcessConnection allocateBoundConnectionForTesting(Context context,
ChildProcessCreationParams creationParams) {
return allocateBoundConnection(
new ChildSpawnData(context, null /* commandLine */, 0 /* childProcessId */,
null /* filesToBeMapped */, null /* LaunchCallback */,
null /* childProcessCallback */, true /* inSandbox */,
false /* alwaysInForeground */, creationParams),
null /* startCallback */, false /* forWarmUp */);
}

@VisibleForTesting
static ChildProcessConnection allocateConnectionForTesting(
Context context, ChildProcessCreationParams creationParams) {
return allocateConnection(
new ChildSpawnData(context, null /* commandLine */, 0 /* childProcessId */,
null /* filesToBeMapped */, null /* launchCallback */,
null /* childProcessCallback */, true /* inSandbox */,
false /* alwaysInForeground */, creationParams),
createCommonParamsBundle(creationParams), false /* forWarmUp */);
}

/**
* Queue up a spawn requests for testing.
*/
@VisibleForTesting
static void enqueuePendingSpawnForTesting(Context context, String[] commandLine,
ChildProcessCreationParams creationParams, boolean inSandbox) {
String packageName = creationParams != null ? creationParams.getPackageName()
: context.getPackageName();
ChildConnectionAllocator allocator =
ChildConnectionAllocator.getAllocator(context, packageName, inSandbox);
allocator.enqueuePendingQueueForTesting(new ChildSpawnData(context, commandLine,
1 /* childProcessId */, new FileDescriptorInfo[0], null /* launchCallback */,
null /* childProcessCallback */, true /* inSandbox */,
false /* alwaysInForeground */, creationParams));
}

/**
* @return the number of sandboxed connections of given {@link packageName} managed by the
* allocator.
*/
@VisibleForTesting
static int allocatedSandboxedConnectionsCountForTesting(Context context, String packageName) {
return ChildConnectionAllocator.getAllocator(context, packageName, true /*isSandboxed */)
.allocatedConnectionsCountForTesting();
}

/**
* @return gets the service connection array for a specific package name.
*/
@VisibleForTesting
static ChildProcessConnection[] getSandboxedConnectionArrayForTesting(
Context context, String packageName) {
return ChildConnectionAllocator.getAllocator(context, packageName, true /*isSandboxed */)
.connectionArrayForTesting();
}

/** @return the count of services set up and working */
@VisibleForTesting
static int connectedServicesCountForTesting() {
return sServiceMap.size();
}

/**
* @param context The context.
* @param packageName The package name of the {@link ChildProcessAlocator}.
* @param inSandbox Whether the connection is sandboxed.
* @return the count of pending spawns in the queue.
*/
@VisibleForTesting
static int pendingSpawnsCountForTesting(
Context context, String packageName, boolean inSandbox) {
return ChildConnectionAllocator.getAllocator(context, packageName, inSandbox)
.pendingSpawnsCountForTesting();
}

/**
* Kills the child process for testing.
* @return true iff the process was killed as expected
Expand Down
8 changes: 5 additions & 3 deletions content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ class ChildProcessLauncherHelper {
private long mNativeChildProcessLauncherHelper;
private int mPid;

// Called on launcher thread.
@CalledByNative
private static FileDescriptorInfo makeFdInfo(
int id, int fd, boolean autoClose, long offset, long size) {
assert LauncherThread.runningOnLauncherThread();
ParcelFileDescriptor pFd;
if (autoClose) {
// Adopt the FD, it will be closed when we close the ParcelFileDescriptor.
Expand All @@ -48,17 +48,18 @@ private static FileDescriptorInfo makeFdInfo(
return new FileDescriptorInfo(id, pFd, offset, size);
}

// Called on launcher thread.
@CalledByNative
private static ChildProcessLauncherHelper create(long nativePointer, Context context,
int paramId, final String[] commandLine, int childProcessId,
FileDescriptorInfo[] filesToBeMapped) {
assert LauncherThread.runningOnLauncherThread();
return new ChildProcessLauncherHelper(
nativePointer, context, paramId, commandLine, childProcessId, filesToBeMapped);
}

private ChildProcessLauncherHelper(long nativePointer, Context context, int paramId,
final String[] commandLine, int childProcessId, FileDescriptorInfo[] filesToBeMapped) {
assert LauncherThread.runningOnLauncherThread();
mNativeChildProcessLauncherHelper = nativePointer;

ChildProcessLauncher.start(context, paramId, commandLine, childProcessId, filesToBeMapped,
Expand All @@ -80,13 +81,14 @@ private boolean isOomProtected() {
return ChildProcessLauncher.getBindingManager().isOomProtected(mPid);
}

// Called on launcher thread.
@CalledByNative
private void setInForeground(int pid, boolean inForeground) {
assert LauncherThread.runningOnLauncherThread();
assert mPid == pid;
ChildProcessLauncher.getBindingManager().setInForeground(mPid, inForeground);
}

// Called on client (UI or IO) thread and launcher thread.
@CalledByNative
private static void stop(int pid) {
ChildProcessLauncher.stop(pid);
Expand Down
10 changes: 8 additions & 2 deletions content/public/android/java/src/org/chromium/content/browser/LauncherThread.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
package org.chromium.content.browser;

import android.os.Handler;
import android.os.Looper;

import org.chromium.base.JavaHandlerThread;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.JNINamespace;

/** This is BrowserThread::PROCESS_LAUNCHER. It is available before native library is loaded. */
@JNINamespace("content::android")
final class LauncherThread {
public final class LauncherThread {
private static final JavaHandlerThread sThread =
new JavaHandlerThread("Chrome_ProcessLauncherThread");
private static final Handler sHandler;
Expand All @@ -20,10 +22,14 @@ final class LauncherThread {
sHandler = new Handler(sThread.getLooper());
}

static void post(Runnable r) {
public static void post(Runnable r) {
sHandler.post(r);
}

public static boolean runningOnLauncherThread() {
return sHandler.getLooper() == Looper.myLooper();
}

@CalledByNative
private static JavaHandlerThread getHandlerThread() {
return sThread;
Expand Down
Loading

0 comments on commit 06114d7

Please sign in to comment.