Skip to content

Commit

Permalink
Revert on M57 "Bind Android ChildProcessServices to a specific client…
Browse files Browse the repository at this point in the history 
… PID."

Suspect cause of crbug.com/690118

> Bind Android ChildProcessServices to a specific client PID.
>
> A ChildProcessService may only be setup once, but nothing enforced this limit,
> and a second client of the service would receive no indication of failure. This
> situation could occur with Android WebView in multi-process mode, if two
> android:processes in the same package tried to use WebView.
>
> To solve this issue, introduce an initial handshake message in
> IChildProcessService that must be issued before setupConnection. This method
> binds the service to the calling PID. If that binding fails, the
> ChildProcessLauncher can then re-try creating a service process using the
> next available slot.
>
> BUG=558377,683133
>
> Review-Url: https://codereview.chromium.org/2626413004
> Cr-Commit-Position: refs/heads/master@{#445779}
> (cherry picked from commit abd3ee7)
>
> Review-Url: https://codereview.chromium.org/2650013003 .
> Cr-Commit-Position: refs/branch-heads/2987@{nwjs#68}
> Cr-Branched-From: ad51088-refs/heads/master@{#444943}

BUG=690118

Review-Url: https://codereview.chromium.org/2695083002 .
Cr-Commit-Position: refs/branch-heads/2987@{#502}
Cr-Branched-From: ad51088-refs/heads/master@{#444943}
  • Loading branch information
Bo Liu committed Feb 14, 2017
1 parent fa139ac commit fd99e10
Show file tree
Hide file tree
Showing 11 changed files with 28 additions and 437 deletions.
39 changes: 3 additions & 36 deletions content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,7 @@
public class ChildProcessServiceImpl {
private static final String MAIN_THREAD_NAME = "ChildProcessMain";
private static final String TAG = "ChildProcessService";

// Lock that protects the following members.
private final Object mBinderLock = new Object();
private IChildProcessCallback mCallback;
// PID of the client of this service, set in bindToCaller().
private int mBoundCallingPid;

// This is the native "Main" thread for the renderer / utility process.
private Thread mMainThread;
Expand Down Expand Up @@ -101,39 +96,11 @@ private Linker getLinker() {
// Binder object used by clients for this service.
private final IChildProcessService.Stub mBinder = new IChildProcessService.Stub() {
// NOTE: Implement any IChildProcessService methods here.
@Override
public boolean bindToCaller() {
synchronized (mBinderLock) {
int callingPid = Binder.getCallingPid();
if (mBoundCallingPid == 0) {
mBoundCallingPid = callingPid;
} else if (mBoundCallingPid != callingPid) {
Log.e(TAG, "Service is already bound by pid %d, cannot bind for pid %d",
mBoundCallingPid, callingPid);
return false;
}
}
return true;
}

@Override
public int setupConnection(Bundle args, IChildProcessCallback callback) {
int callingPid = Binder.getCallingPid();
synchronized (mBinderLock) {
if (mBoundCallingPid != callingPid) {
if (mBoundCallingPid == 0) {
Log.e(TAG, "Service has not been bound with bindToCaller()");
} else {
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
mBoundCallingPid);
}
return -1;
}

mCallback = callback;
getServiceInfo(args);
return Process.myPid();
}
mCallback = callback;
getServiceInfo(args);
return Process.myPid();
}

@Override
Expand Down
21 changes: 1 addition & 20 deletions content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,6 @@ interface DeathCallback {
void onChildProcessDied(ChildProcessConnection connection);
}

/**
* Used to notify the consumer about the process start. These callbacks will be invoked before
* the ConnectionCallbacks.
*/
interface StartCallback {
/**
* Called when the child process has successfully started and is ready for connection
* setup.
*/
void onChildStarted();

/**
* Called when the child process failed to start. This can happen if the process is already
* in use by another client.
*/
void onChildStartFailed();
}

/**
* Used to notify the consumer about the connection being established.
*/
Expand Down Expand Up @@ -75,9 +57,8 @@ interface ConnectionCallback {
* remainder later while reducing the connection setup latency.
* @param commandLine (optional) command line for the child process. If omitted, then
* the command line parameters must instead be passed to setupConnection().
* @param startCallback (optional) callback when the child process starts or fails to start.
*/
void start(String[] commandLine, StartCallback startCallback);
void start(String[] commandLine);

/**
* Setups the connection after it was started with start().
Expand Down
22 changes: 1 addition & 21 deletions content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,6 @@ private static class ConnectionParams {
}
}

// This is set in start() and is used in onServiceConnected().
private ChildProcessConnection.StartCallback mStartCallback;

// This is set in setupConnection() and is later used in doConnectionSetupLocked(), after which
// the variable is cleared. Therefore this is only valid while the connection is being set up.
private ConnectionParams mConnectionParams;
Expand Down Expand Up @@ -171,21 +168,6 @@ public void onServiceConnected(ComponentName className, IBinder service) {
"ChildProcessConnectionImpl.ChildServiceConnection.onServiceConnected");
mServiceConnectComplete = true;
mService = IChildProcessService.Stub.asInterface(service);

boolean boundToUs = false;
try {
boundToUs = mService.bindToCaller();
} catch (RemoteException ex) {
}
if (!boundToUs) {
if (mStartCallback != null) {
mStartCallback.onChildStartFailed();
}
return;
} else if (mStartCallback != null) {
mStartCallback.onChildStarted();
}

// Run the setup if the connection parameters have already been provided. If
// not, doConnectionSetupLocked() will be called from setupConnection().
if (mConnectionParams != null) {
Expand Down Expand Up @@ -307,16 +289,14 @@ public int getPid() {
}

@Override
public void start(String[] commandLine, ChildProcessConnection.StartCallback startCallback) {
public void start(String[] commandLine) {
try {
TraceEvent.begin("ChildProcessConnectionImpl.start");
synchronized (mLock) {
assert !ThreadUtils.runningOnUiThread();
assert mConnectionParams == null :
"setupConnection() called before start() in ChildProcessConnectionImpl.";

mStartCallback = startCallback;

if (!mInitialBinding.bind(commandLine)) {
Log.e(TAG, "Failed to establish the service connection.");
// We have to notify the caller so that they can free-up associated resources.
Expand Down
113 changes: 17 additions & 96 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 @@ -8,7 +8,6 @@
import android.content.Context;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
import android.os.AsyncTask;
import android.os.Bundle;
import android.os.ParcelFileDescriptor;
import android.os.RemoteException;
Expand Down Expand Up @@ -414,13 +413,12 @@ private static ChromiumLinkerParams getLinkerParamsForNewConnection() {

private static ChildProcessConnection allocateBoundConnection(Context context,
String[] commandLine, boolean inSandbox, boolean alwaysInForeground,
ChildProcessCreationParams creationParams,
ChildProcessConnection.StartCallback startCallback) {
ChildProcessCreationParams creationParams) {
ChromiumLinkerParams chromiumLinkerParams = getLinkerParamsForNewConnection();
ChildProcessConnection connection = allocateConnection(
context, inSandbox, chromiumLinkerParams, alwaysInForeground, creationParams);
if (connection != null) {
connection.start(commandLine, startCallback);
connection.start(commandLine);

String packageName = creationParams != null ? creationParams.getPackageName()
: context.getPackageName();
Expand All @@ -438,7 +436,7 @@ private static ChildProcessConnection allocateBoundConnection(Context context,
private static final long FREE_CONNECTION_DELAY_MILLIS = 1;

private static void freeConnection(ChildProcessConnection connection) {
synchronized (sSpareConnectionLock) {
synchronized (ChildProcessLauncher.class) {
if (connection.equals(sSpareSandboxedConnection)) sSpareSandboxedConnection = null;
}

Expand Down Expand Up @@ -485,16 +483,8 @@ private static PendingSpawnData freeConnectionAndDequeuePending(ChildProcessConn
private static Map<Integer, ChildProcessConnection> sServiceMap =
new ConcurrentHashMap<Integer, ChildProcessConnection>();

// Lock and monitor for these members {{{
private static final Object sSpareConnectionLock = new Object();
// A pre-allocated and pre-bound connection ready for connection setup, or null.
private static ChildProcessConnection sSpareSandboxedConnection;
// If sSpareSandboxedConnection is not null, this indicates whether the service is
// ready for connection setup. Wait on the monitor lock to be notified when this
// state changes. sSpareSandboxedConnection may be null after waiting, if starting
// the service failed.
private static boolean sSpareConnectionStarting;
// }}}

// Manages oom bindings used to bind chind services.
private static BindingManager sBindingManager = BindingManagerImpl.createBindingManager();
Expand Down Expand Up @@ -577,38 +567,15 @@ static boolean isApplicationInForeground() {
* @param context the application context used for the connection.
*/
public static void warmUp(Context context) {
synchronized (sSpareConnectionLock) {
synchronized (ChildProcessLauncher.class) {
assert !ThreadUtils.runningOnUiThread();
if (sSpareSandboxedConnection == null) {
ChildProcessCreationParams params = ChildProcessCreationParams.get();
if (params != null) {
params = params.copy();
}

sSpareConnectionStarting = true;

ChildProcessConnection.StartCallback startCallback =
new ChildProcessConnection.StartCallback() {
@Override
public void onChildStarted() {
synchronized (sSpareConnectionLock) {
sSpareConnectionStarting = false;
sSpareConnectionLock.notify();
}
}

@Override
public void onChildStartFailed() {
Log.e(TAG, "Failed to warm up the spare sandbox service");
synchronized (sSpareConnectionLock) {
sSpareSandboxedConnection = null;
sSpareConnectionStarting = false;
sSpareConnectionLock.notify();
}
}
};
sSpareSandboxedConnection = allocateBoundConnection(context, null, true, false,
params, startCallback);
params);
}
}
}
Expand Down Expand Up @@ -686,30 +653,24 @@ private static void start(Context context, final String[] commandLine, int child
callbackType, inSandbox, params);
}

private static ChildProcessConnection startInternal(
final Context context,
private static void startInternal(
Context context,
final String[] commandLine,
final int childProcessId,
final FileDescriptorInfo[] filesToBeMapped,
final long clientContext,
final int callbackType,
final boolean inSandbox,
final ChildProcessCreationParams creationParams) {
int childProcessId,
FileDescriptorInfo[] filesToBeMapped,
long clientContext,
int callbackType,
boolean inSandbox,
ChildProcessCreationParams creationParams) {
try {
TraceEvent.begin("ChildProcessLauncher.startInternal");

ChildProcessConnection allocatedConnection = null;
String packageName = creationParams != null ? creationParams.getPackageName()
: context.getPackageName();
synchronized (sSpareConnectionLock) {
synchronized (ChildProcessLauncher.class) {
if (inSandbox && sSpareSandboxedConnection != null
&& sSpareSandboxedConnection.getPackageName().equals(packageName)) {
while (sSpareConnectionStarting) {
try {
sSpareConnectionLock.wait();
} catch (InterruptedException ex) {
}
}
allocatedConnection = sSpareSandboxedConnection;
sSpareSandboxedConnection = null;
}
Expand All @@ -719,39 +680,15 @@ private static ChildProcessConnection startInternal(
if (callbackType == CALLBACK_FOR_GPU_PROCESS) alwaysInForeground = true;
PendingSpawnQueue pendingSpawnQueue = getPendingSpawnQueue(
context, packageName, inSandbox);
ChildProcessConnection.StartCallback startCallback =
new ChildProcessConnection.StartCallback() {
@Override
public void onChildStarted() {}

@Override
public void onChildStartFailed() {
Log.e(TAG, "ChildProcessConnection.start failed, trying again");
AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
@Override
public void run() {
// The child process may already be bound to another client
// (this can happen if multi-process WebView is used in more
// than one process), so try starting the process again.
// This connection that failed to start has not been freed,
// so a new bound connection will be allocated.
startInternal(context, commandLine, childProcessId,
filesToBeMapped, clientContext, callbackType,
inSandbox, creationParams);
}
});
}
};
synchronized (pendingSpawnQueue.mPendingSpawnsLock) {
allocatedConnection = allocateBoundConnection(
context, commandLine, inSandbox, alwaysInForeground, creationParams,
startCallback);
context, commandLine, inSandbox, alwaysInForeground, creationParams);
if (allocatedConnection == null) {
Log.d(TAG, "Allocation of new service failed. Queuing up pending spawn.");
pendingSpawnQueue.enqueueLocked(new PendingSpawnData(context, commandLine,
childProcessId, filesToBeMapped, clientContext,
callbackType, inSandbox, creationParams));
return null;
return;
}
}
}
Expand All @@ -760,7 +697,6 @@ public void run() {
allocatedConnection.getServiceNumber());
triggerConnectionSetup(allocatedConnection, commandLine, childProcessId,
filesToBeMapped, callbackType, clientContext);
return allocatedConnection;
} finally {
TraceEvent.end("ChildProcessLauncher.startInternal");
}
Expand Down Expand Up @@ -897,17 +833,10 @@ static void logPidWarning(int pid, String message) {
}
}

@VisibleForTesting
public static ChildProcessConnection startForTesting(Context context, String[] commandLine,
FileDescriptorInfo[] filesToMap, ChildProcessCreationParams params) {
return startInternal(context, commandLine, 0, filesToMap, 0,
CALLBACK_FOR_RENDERER_PROCESS, true, params);
}

@VisibleForTesting
static ChildProcessConnection allocateBoundConnectionForTesting(Context context,
ChildProcessCreationParams creationParams) {
return allocateBoundConnection(context, null, true, false, creationParams, null);
return allocateBoundConnection(context, null, true, false, creationParams);
}

@VisibleForTesting
Expand Down Expand Up @@ -945,14 +874,6 @@ static int allocatedSandboxedConnectionsCountForTesting(Context context, String
.allocatedConnectionsCountForTesting();
}

/**
* @return the service map of connected services
*/
@VisibleForTesting
static Map<Integer, ChildProcessConnection> getServiceMapForTesting() {
return sServiceMap;
}

/** @return the count of services set up and working */
@VisibleForTesting
static int connectedServicesCountForTesting() {
Expand Down
5 changes: 0 additions & 5 deletions content/public/android/java/src/org/chromium/content/common/IChildProcessService.aidl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ import android.view.Surface;
import android.os.Bundle;

interface IChildProcessService {
// On the first call to this method, the service will record the calling PID
// and return true. Subsequent calls will only return true if the calling PID
// is the same as the recorded one.
boolean bindToCaller();

// Sets up the initial IPC channel and returns the pid of the child process.
int setupConnection(in Bundle args, IChildProcessCallback callback);

Expand Down
2 changes: 0 additions & 2 deletions content/public/android/java/src/org/chromium/content/common/OWNERS
View file

This file was deleted.

Loading

0 comments on commit fd99e10

Please sign in to comment.