Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017963003:
Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. (Closed)
DescriptionUpstream: ChildProcessLauncher connects renderer processes of WebAPKs.
A refactoring is done in ChildProcessLauncher to have a map for inSandboxed
ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned
to a different ChildConnectionAllocator depending on the package name.
We use ChildProcessCreationParams to plumb in WebAPK's package name into
ChildProcessLauncher. So the allocator knows which application's renderer
services to connect, and the maximum number of services that are allowed
by that application.
Always initialize ChildProcessCreationParams.
BUG=609122
Committed: https://crrev.com/93b503c8708027a5943cdbc76e6fc455c09ef4f2
Cr-Commit-Position: refs/heads/master@{#398801}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Fix the MockChildProcessConnection. #
Total comments: 17
Patch Set 3 : pkotwicz@'s comments. #Patch Set 4 : Each Allocator has a PendingSpawnQueue and rebase. #
Total comments: 1
Patch Set 5 : Nits #Patch Set 6 : Remove the assumption that ChildProcessCreationParams is always non null. #
Total comments: 17
Patch Set 7 : Maria's comments. #
Total comments: 1
Patch Set 8 : Revert and use ConcurrentHashMap. #
Total comments: 2
Patch Set 9 : Add a TODO. #
Total comments: 2
Patch Set 10 : Remove PROCESS_WEBAPK_CHILD. #Messages
Total messages: 59 (30 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. BUG=609122 ========== to ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services are allowed of that application. Always initialize ChildProcessCreationParams in every code path. BUG=609122 ==========
Description was changed from ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services are allowed of that application. Always initialize ChildProcessCreationParams in every code path. BUG=609122 ========== to ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams in every code path. BUG=609122 ==========
Description was changed from ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams in every code path. BUG=609122 ========== to ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams. BUG=609122 ==========
Patchset #1 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
hanxi@chromium.org changed reviewers: + michaelbai@chromium.org
hanxi@chromium.org changed reviewers: - michaelbai@chromium.org
https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: // {@link ChildProcessLauncher} knows which application's renderer service to connect. Nit: connect to Can you update this comment. I do not understand what the "WebAPK child process creation parameter" is. https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:59: // window or with --site-per-process enabled. window -> windows https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:68: Nit: isForWebApk Please add a comment to this function https://codereview.chromium.org/2017963003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2017963003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:44: return new ChildProcessCreationParams(mPackageName, mExtraBindFlags, mLibraryProcessType); Can you please check that there are no errors thrown when compiling with run_findbugs=true
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: // {@link ChildProcessLauncher} knows which application's renderer service to connect. On 2016/05/27 21:10:13, pkotwicz wrote: > Nit: connect to > > Can you update this comment. I do not understand what the "WebAPK child process > creation parameter" is. Done. https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:59: // window or with --site-per-process enabled. On 2016/05/27 21:10:13, pkotwicz wrote: > window -> windows Done. https://codereview.chromium.org/2017963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:68: On 2016/05/27 21:10:13, pkotwicz wrote: > Nit: isForWebApk > > Please add a comment to this function Done. https://codereview.chromium.org/2017963003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2017963003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:44: return new ChildProcessCreationParams(mPackageName, mExtraBindFlags, mLibraryProcessType); On 2016/05/27 21:10:13, pkotwicz wrote: > Can you please check that there are no errors thrown when compiling with > run_findbugs=true Good catch. Rename it to copy().
Thank you for putting this CL together. A few comments https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:130: /** Returns the count of connections managed by the allocator */ Why this change? https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:211: /** Returns the count of pending spawns in the queue */ Why this change? https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:361: && !sSandboxedChildConnectionAllocatorMap.get(creationParams.getPackageName()) Nit: Use getConnectionAllocator() instead of sSandboxedChildConnectionAllocatorMap https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:396: pendingSpawn.inSandbox(), conn.getCreationParams()); The params of the freed connection may be different from the params of the pending connection https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:441: /** Returns true iff the child process is protected from out-of-memory killing */ Why this change? https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:546: Context context, boolean moderateBindingTillBackgrounded) { {@link sBindingManager} is a singleton. This means that: - WebAPKs and non WebAPKs shared the same moderate binding pool (I think this is ok) - The size of the shared moderate binding pool is set based on the number of sandboxes processes used by Chrome. This is very confusing. This should have an explanatory comment. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:690: sSpareSandboxedConnection = null; We may be wanting to create a connection for a WebAPK but the spare connection may be for a non-WebAPK or vice versa. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:700: Log.d(TAG, "Allocation of new service failed. Queuing up pending spawn."); This is suboptimal. There should be a {@link sPendingSpawnQueue} per allocator. Ideally if: - Chrome creates one sandboxed process - If a WebAPK attempts to create 4 sandboxed processes (This should cause 1 ChildProcessConnection to be queued) - Chrome sandboxed process is killed ChildProcessLauncher will not try to start the 4th sandbox process (the number of sandboxed processes is limited per package name)
Patchset #4 (id:110001) has been deleted
Patchset #4 (id:130001) has been deleted
Patchset #4 (id:150001) has been deleted
Patchset #4 (id:170001) has been deleted
Patchset #4 (id:190001) has been deleted
Patchset #4 (id:210001) has been deleted
Hi Peter, ptal, thanks! https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:130: /** Returns the count of connections managed by the allocator */ On 2016/05/28 01:04:14, pkotwicz wrote: > Why this change? Due to the java style guide. I will revert them back since it is easy for reviewing this CL. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:211: /** Returns the count of pending spawns in the queue */ On 2016/05/28 01:04:14, pkotwicz wrote: > Why this change? Done. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:361: && !sSandboxedChildConnectionAllocatorMap.get(creationParams.getPackageName()) On 2016/05/28 01:04:14, pkotwicz wrote: > Nit: Use getConnectionAllocator() instead of > sSandboxedChildConnectionAllocatorMap Done. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:396: pendingSpawn.inSandbox(), conn.getCreationParams()); On 2016/05/28 01:04:15, pkotwicz wrote: > The params of the freed connection may be different from the params of the > pending connection Good catch. Store ChildProcessCreationParams in PendingSpawnData. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:441: /** Returns true iff the child process is protected from out-of-memory killing */ On 2016/05/28 01:04:15, pkotwicz wrote: > Why this change? Done. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:546: Context context, boolean moderateBindingTillBackgrounded) { On 2016/05/28 01:04:15, pkotwicz wrote: > {@link sBindingManager} is a singleton. This means that: > - WebAPKs and non WebAPKs shared the same moderate binding pool (I think this is > ok) > - The size of the shared moderate binding pool is set based on the number of > sandboxes processes used by Chrome. This is very confusing. > > This should have an explanatory comment. Added a comment here. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:690: sSpareSandboxedConnection = null; On 2016/05/28 01:04:15, pkotwicz wrote: > We may be wanting to create a connection for a WebAPK but the spare connection > may be for a non-WebAPK or vice versa. Add a check here. Only use the SpareSandgoxedConnecion when the package names match. https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:700: Log.d(TAG, "Allocation of new service failed. Queuing up pending spawn."); On 2016/05/28 01:04:15, pkotwicz wrote: > This is suboptimal. There should be a {@link sPendingSpawnQueue} per allocator. > > Ideally if: > - Chrome creates one sandboxed process > - If a WebAPK attempts to create 4 sandboxed processes (This should cause 1 > ChildProcessConnection to be queued) > - Chrome sandboxed process is killed > > ChildProcessLauncher will not try to start the 4th sandbox process (the number > of sandboxed processes is limited per package name) Good catch! Make each allocator owns a PendingSpawnQueue.
LGTM Thank you for making the changes! https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/70001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:130: /** Returns the count of connections managed by the allocator */ My bad. Yes, the Java style guide says you should make this change https://codereview.chromium.org/2017963003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:292: inSandbox, packageName))); Nit: inSandbox -> true
LGTM Thank you for making the changes!
hanxi@chromium.org changed reviewers: + michaelbai@chromium.org
hanxi@chromium.org changed reviewers: + dtrainor@chromium.org - michaelbai@chromium.org
hanxi@chromium.org changed reviewers: - dtrainor@chromium.org
hanxi@chromium.org changed reviewers: + sgurun@chromium.org
hanxi@chromium.org changed reviewers: + torne@chromium.org - sgurun@chromium.org
hanxi@chromium.org changed reviewers: + michaelbai@chromium.org - torne@chromium.org
Hi Tao, could you please take a look? Thanks!
On 2016/05/31 14:08:03, Xi Han wrote: > Hi Tao, could you please take a look? Thanks! One thing I am not sure is whether we should assume ChildProcessCreationParam is always valid. we had a bug crbug.com/612486, it indicated there is race condition between setting ChildProcessCreationParam and creating child process. My fix is temporary because the N sdk isn't released. The final fix might be that we should only set ChildProcessCreationParams if we have to, like Webview and WebAPKs, and use the default value for other cases e.g the package name from context.getPackageName(). Back to your patch, could you - not assume the ChildProcessCreationParams is always set? - not change ChromeApplication and create new ChildProcessCreationParams as need? The best person to review your patch is yfriedman@
Patchset #6 (id:270001) has been deleted
Hi Tao, could you please take another look? Thanks!
https://codereview.chromium.org/2017963003/diff/290001/base/android/library_l... File base/android/library_loader/library_loader_hooks.h (right): https://codereview.chromium.org/2017963003/diff/290001/base/android/library_l... base/android/library_loader/library_loader_hooks.h:29: PROCESS_WEBAPK_CHILD = 5, Do you really need separated type for WebAPK? From this patch, PROCESS_WEBAPK_CHILD is defined and set, isn't used anywhere, you shouldn't have new process type if you don't have to.
https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:670: } michaelbai@ would it be acceptable to set {@link params} to the default value if {@link params} == null. This would save a lot of null checking elsewhere If am thinking: if (params == null) { params = new ChildProcessCreationParams(new ChildProcessCreationParams(context.getPackageName(), 0, LibraryProcessType.PROCESS_CHILD); }
michaelbai@ Ping.
Sorry for late response, I were working on a p0 bug. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:670: } On 2016/06/02 02:16:30, pkotwicz wrote: > michaelbai@ would it be acceptable to set {@link params} to the default value if > {@link params} == null. > This would save a lot of null checking elsewhere > > If am thinking: > > if (params == null) { > params = new ChildProcessCreationParams(new > ChildProcessCreationParams(context.getPackageName(), 0, > LibraryProcessType.PROCESS_CHILD); > } It is fine.
hanxi@chromium.org changed reviewers: + mariakhomenko@chromium.org
Hi Maria, could you please take a look? Thanks! https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:670: } On 2016/06/03 17:38:46, michaelbai wrote: > On 2016/06/02 02:16:30, pkotwicz wrote: > > michaelbai@ would it be acceptable to set {@link params} to the default value > if > > {@link params} == null. > > This would save a lot of null checking elsewhere > > > > If am thinking: > > > > if (params == null) { > > params = new ChildProcessCreationParams(new > > ChildProcessCreationParams(context.getPackageName(), 0, > > LibraryProcessType.PROCESS_CHILD); > > } > > It is fine. Since I already added these checks, will leave them as now unless they bother other reviewers.
https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:73: private PendingSpawnQueue mPendingSpawnQueue = new PendingSpawnQueue(); final? an queue -> a queue. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202: new LinkedList<PendingSpawnData>(); fits on one line https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:203: final Object mPendingSpawnsLock = new Object(); Now that the queue of spawns isn't static, do we still need to lock it? https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:231: // Map from package name to ChildConnectionAllocator; ; -> . https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:278: Context context, boolean inSandbox, String packageName) { it seems like previously this method allocated both a sandboxed allocator and a privileged allocator, but now it will only do either one or the other depending on the value of inSandbox? https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:39: @CommandLineFlags.Add(ChildProcessLauncher.SWITCH_NUM_SANDBOXED_SERVICES_FOR_TESTING + "=4") Why do we need the new flag here? Also doesn't the expectation change here now that we can allocate connections in non-chrome package?
Hi Maria, could you please take another look? Thanks! https://codereview.chromium.org/2017963003/diff/290001/base/android/library_l... File base/android/library_loader/library_loader_hooks.h (right): https://codereview.chromium.org/2017963003/diff/290001/base/android/library_l... base/android/library_loader/library_loader_hooks.h:29: PROCESS_WEBAPK_CHILD = 5, On 2016/06/02 00:14:51, michaelbai wrote: > Do you really need separated type for WebAPK? From this patch, > PROCESS_WEBAPK_CHILD is defined and set, isn't used anywhere, you shouldn't have > new process type if you don't have to. Hmmm, we check the type and did something special for WebAPK when loading the Chrome's shared library in WebAPK. We may be able to find another way to do it, but not 100% sure. I will add a TODO here. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:73: private PendingSpawnQueue mPendingSpawnQueue = new PendingSpawnQueue(); On 2016/06/07 17:20:17, Maria wrote: > final? > > an queue -> a queue. Done. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202: new LinkedList<PendingSpawnData>(); On 2016/06/07 17:20:17, Maria wrote: > fits on one line Done. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:203: final Object mPendingSpawnsLock = new Object(); On 2016/06/07 17:20:17, Maria wrote: > Now that the queue of spawns isn't static, do we still need to lock it? Good catch. Remove the lock. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:231: // Map from package name to ChildConnectionAllocator; On 2016/06/07 17:20:17, Maria wrote: > ; -> . Done. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:278: Context context, boolean inSandbox, String packageName) { On 2016/06/07 17:20:17, Maria wrote: > it seems like previously this method allocated both a sandboxed allocator and a > privileged allocator, but now it will only do either one or the other depending > on the value of inSandbox? Yes, we can prevent the privileged process (GPU) is initialized by using the WebAPK's package name when it is in the WebAPK's run time. The GPU process should always use the default Chrome's parameter if exists. It is possible to use a wrong one, think of the case that WebAPK is launched when Chrome isn't. If the renderer process is created before the GPU process, this function will use the wrong package name to initialize the sPrivilegedChildConnectionAllocator. https://codereview.chromium.org/2017963003/diff/290001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2017963003/diff/290001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:39: @CommandLineFlags.Add(ChildProcessLauncher.SWITCH_NUM_SANDBOXED_SERVICES_FOR_TESTING + "=4") On 2016/06/07 17:20:17, Maria wrote: > Why do we need the new flag here? > > Also doesn't the expectation change here now that we can allocate connections in > non-chrome package? This is because ChildProcessLauncher#getNumberOfServices throws exception when the NUM_SANDBOXED_SERVICES isn't declared in AndroidManifest.xml file. Since in this function we use a fake context (getInstrumentation().getContext()), when the getNumberOfServices() is called, an exception will be thrown. So we use this flag to bypass the check within getNumberOfServices().
Patchset #7 (id:310001) has been deleted
https://codereview.chromium.org/2017963003/diff/330001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/330001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429: return pendingSpawnQueue.dequeue(); So, I think the comment you deleted from the spawn queue lock mentioned that operations on the queue have to be atomic wrt free connection updates, which they no longer are in this version. Unfortunately this code is very hard to reason about and I am not as familiar with it as I'd like. We just need to be very careful to ensure we don't introduce new synchronization issues with this patch. I am not quite sure whether we need to keep some sort of lock on the queue or if we should just put this operation inside mConnectionLock block within the allocator. @michaelbai, do you know this code well enough to tell?
On 2016/06/07 20:41:36, Maria wrote: > https://codereview.chromium.org/2017963003/diff/330001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > (right): > > https://codereview.chromium.org/2017963003/diff/330001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429: > return pendingSpawnQueue.dequeue(); > So, I think the comment you deleted from the spawn queue lock mentioned that > operations on the queue have to be atomic wrt free connection updates, which > they no longer are in this version. > > Unfortunately this code is very hard to reason about and I am not as familiar > with it as I'd like. We just need to be very careful to ensure we don't > introduce new synchronization issues with this patch. > > I am not quite sure whether we need to keep some sort of lock on the queue or if > we should just put this operation inside mConnectionLock block within the > allocator. > > @michaelbai, do you know this code well enough to tell? Ok, I discussed the concurrency story here with dtrainor@ and nyquist@. Here's what we concluded: 1) You need to reinstate the locking inside PendingSpawnQueue (sorry for misleading you about this one -- that lock is necessary and your code changing it to non-static was correct) 2) You need to make sSandboxedChildConnectionAllocatorMap accesses thread safe. There are two ways to do so. The first way is to switch to using a ConcurrentHashMap. The second way is to guard every .put() .get() and .size() call on the map with a lock. Currently the put is guarded and get is not which may lead to problems if the map is accessed from different threads.
Patchset #8 (id:350001) has been deleted
Thank you Maria for all the suggestions. I added the lock back and use ConcurrentHashMap for the SandboxedProcessAlocatorMap. PTAL, thanks!
lgtm https://codereview.chromium.org/2017963003/diff/370001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/370001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:277: synchronized (ChildProcessLauncher.class) { Please add a TODO with my name here to move to using an object to lock the access. It came up yesterday in our discussion and I know it's already this way, but it should probably be changed in the future.
hanxi@chromium.org changed reviewers: + dtrainor@chromium.org
Hi David, I need OWNERS review for the following classes: - content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java - content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java Could you please take a look? Thanks! https://codereview.chromium.org/2017963003/diff/370001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2017963003/diff/370001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:277: synchronized (ChildProcessLauncher.class) { On 2016/06/08 18:25:08, Maria wrote: > Please add a TODO with my name here to move to using an object to lock the > access. It came up yesterday in our discussion and I know it's already this way, > but it should probably be changed in the future. Done.
https://codereview.chromium.org/2017963003/diff/390001/base/android/library_l... File base/android/library_loader/library_loader_hooks.h (right): https://codereview.chromium.org/2017963003/diff/390001/base/android/library_l... base/android/library_loader/library_loader_hooks.h:31: PROCESS_WEBAPK_CHILD = 5, Sorry, could you add it back when you really use it, it is here to make someone wonder how it is used, and it is nice if you add and use it in one CL later.
https://codereview.chromium.org/2017963003/diff/390001/base/android/library_l... File base/android/library_loader/library_loader_hooks.h (right): https://codereview.chromium.org/2017963003/diff/390001/base/android/library_l... base/android/library_loader/library_loader_hooks.h:31: PROCESS_WEBAPK_CHILD = 5, On 2016/06/08 19:27:12, michaelbai wrote: > Sorry, could you add it back when you really use it, it is here to make someone > wonder how it is used, and it is nice if you add and use it in one CL later. Sure, removed.
Thanks, LGTM, I only reviewed ChildProcessCreationParams related code.
ChildProcessCreationParams.java and BindingManagerImplTest.java lgtm
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2017963003/#ps410001 (title: "Remove PROCESS_WEBAPK_CHILD.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2017963003/410001
Message was sent while issue was closed.
Description was changed from ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams. BUG=609122 ========== to ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams. BUG=609122 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams. BUG=609122 ========== to ========== Upstream: ChildProcessLauncher connects renderer processes of WebAPKs. A refactoring is done in ChildProcessLauncher to have a map for inSandboxed ChildConnectionAllocators. It allows WebAPKs' renderer services are assigned to a different ChildConnectionAllocator depending on the package name. We use ChildProcessCreationParams to plumb in WebAPK's package name into ChildProcessLauncher. So the allocator knows which application's renderer services to connect, and the maximum number of services that are allowed by that application. Always initialize ChildProcessCreationParams. BUG=609122 Committed: https://crrev.com/93b503c8708027a5943cdbc76e6fc455c09ef4f2 Cr-Commit-Position: refs/heads/master@{#398801} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/93b503c8708027a5943cdbc76e6fc455c09ef4f2 Cr-Commit-Position: refs/heads/master@{#398801} |
