android: Remove command line args from intent bundle

content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.content.app;
 
 import android.content.Context;
 import android.content.Intent;
 import android.graphics.SurfaceTexture;
 import android.os.Binder;
@@ skipping 147 matching lines @@
                         while (mCommandLineParams == null) {
                             mMainThread.wait();
                         }
                     }
                     CommandLine.init(mCommandLineParams);
 
                     Linker linker = null;
                     boolean requestedSharedRelro = false;
                     if (Linker.isUsed()) {
                         synchronized (mMainThread) {
                             while (!mIsBound) {

[Jay Civelli, 2016/12/12 17:40:28]

I guess now we'll always be bound by the time we get there?

[boliu, 2016/12/12 17:54:02]

Yeah afaict, so should be safe to remove.

And removing should make the regression slightly less (though I guess probably
not by much).

Others got opinion whether removing this is safe?

[boliu, 2016/12/12 18:13:08]

Actually, I can't find any hard evidence to back this up.

If onBind *always* happens before setupConnection, then this mIsBound check is
not needed even without this CL, which leads me to think when digit@ first wrote
this, that setupConnection before bind *is* possible.

So to be cautious for merging, let's not touch this..

[boliu, 2016/12/12 18:21:22]

Oh, bahh, I can't decide.. Here's the evidence:
bind returns the aidl interface that browser then uses to call setupConnection,
so bind has to happen first.

Also nothing ever calls ChildProcessService.initializeParams directly, so the
only callsite of initializeParams is from bind.

                                 mMainThread.wait();
                             }
                         }
                         linker = getLinker();
                         if (mLinkerParams.mWaitForSharedRelro) {
                             requestedSharedRelro = true;
                             linker.initServiceProcess(mLinkerParams.mBaseLoadAddress);
                         } else {
                             linker.disableSharedRelros();
                         }
@@ skipping 98 matching lines @@
      */
     @UsedByReflection("WebApkSandboxedProcessService")
     public IBinder bind(Intent intent, int authorizedCallerUid) {
         mAuthorizedCallerUid = authorizedCallerUid;
         initializeParams(intent);
         return mBinder;
     }
 
     void initializeParams(Intent intent) {
         synchronized (mMainThread) {
-            mCommandLineParams =
-                    intent.getStringArrayExtra(ChildProcessConstants.EXTRA_COMMAND_LINE);
             // mLinkerParams is never used if Linker.isUsed() returns false.
             // See onCreate().
             mLinkerParams = new ChromiumLinkerParams(intent);

[Jay Civelli, 2016/12/12 17:40:28]

In the future, should we still depend on anything from the intent at all?

[boliu, 2016/12/12 17:54:02]

I dunno.. it's still ok to pass things that's fixed for the entire installation.
And if linker args fall into that category, then it's a win on start up time

[Jay Civelli, 2016/12/12 19:11:22]

Yes, but then the linker has to wait for the command line to be set iiuc. I
think it would be less error prone to always pass everything in the IPC call.
Not something you need to do there, but maybe add a TODO. When I'll refactor
this code as part of the servicification effort, I'll take a look at this.

[boliu, 2016/12/12 19:16:02]

Surely we can refactor the linker to not require command line args? which of
course is out of scope for this CL. But that's my plan if perf bots complain.

             mLibraryProcessType = ChildProcessCreationParams.getLibraryProcessType(intent);
             mIsBound = true;
             mMainThread.notifyAll();
         }
     }
 
     void getServiceInfo(Bundle bundle) {
         // Required to unparcel FileDescriptorInfo.
         bundle.setClassLoader(mHostClassLoader);
         synchronized (mMainThread) {
             // Allow the command line to be set via bind() intent or setupConnection, but

[Jay Civelli, 2016/12/12 17:40:28]

Is this comment still accurate?

[boliu, 2016/12/12 17:54:02]

I dunno what this comment is really saying.. bind always comes first afaik, so
it's just saying command line args is (was) in bind, but transferring FDs has to
be here (obviously..)

Uhh... just remove it?

[Jay Civelli, 2016/12/12 19:11:22]

Yes, maybe remove since now there is only one way to set the cmd line now.

             // the FD can only be transferred here.
             if (mCommandLineParams == null) {
                 mCommandLineParams =
                         bundle.getStringArray(ChildProcessConstants.EXTRA_COMMAND_LINE);
+                mMainThread.notifyAll();
             }
             // We must have received the command line by now
             assert mCommandLineParams != null;
             mCpuCount = bundle.getInt(ChildProcessConstants.EXTRA_CPU_COUNT);
             mCpuFeatures = bundle.getLong(ChildProcessConstants.EXTRA_CPU_FEATURES);
             assert mCpuCount > 0;
             Parcelable[] fdInfosAsParcelable =
                     bundle.getParcelableArray(ChildProcessConstants.EXTRA_FILES);
             if (fdInfosAsParcelable != null) {
                 // For why this arraycopy is necessary:
@@ skipping 108 matching lines @@
     private static native void nativeInitChildProcessImpl(
             ChildProcessServiceImpl serviceImpl, int cpuCount, long cpuFeatures);
 
     /**
      * Force the child process to exit.
      */
     private static native void nativeExitChildProcess();
 
     private native void nativeShutdownMainThread();
 }