Removes the setting of ChildProcessCreationParams in ChildProcessLauncher#start() after N sdk is released. |
||||||
Issue descriptionCL to introduce the setting is: https://codereview.chromium.org/2063213002/ We should remove the embedder implementation details of ChildProcessCreationParams from the content-layer in ChildProcessLauncher#start(). Some details: Context.BIND_EXTERNAL_SERVICE is required for exported service on N+ (see https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java?rcl=0&l=233). Before N sdk is released, we cannot set this flag directly in {@link ChildProcessConnectionImpl}, but hides in the clank repository. Fails to set this flag, in other words, without creating a ChildProcessCreationParams, causes the crash of crbug.com/619563. For child process, renderer process is treated differently with other child process(e.g. GPU and utility process). This is because WebView and WebAPKs have renderer processes running in their application. When launching these renderer processes, {@link ChildProcessConnectionImpl} requires the package name of the application which holds the renderer process. Therefore, ChildProcessCreationParams was introduced to plumb in the package name of the host application. However, for other child process type, the package name is always be Chrome' package: - renderers: package name in ChildProcessCreationParams could be the WebView/WebAPK/Chrome's package, depending on the host application of a renderer process. - GPU/Utility processes: always be Chrome's package name. The "number of service" is also treated differently for renderers or other child processes: - renderers: getNumberOfServices() would look up the host application's AndroidManifest.xml to find "NUM_SANDBOXED_SERVICES" declared. The host application could be WebView/WebAPKs/Chrome. - GPU/Utility processes: getNumberOfServices() returns the "NUM_PRIVILEGED_SERVICES" in Chrome's AndroidManifest.xml. We should also figure out a way to better define the above behavior.
,
Jan 25 2017
,
May 11 2017
Looking at this a bit. I want to get to this state: * Make no distinction sandboxed vs privileged services. If sandboxed needs to be external/exported, then let's make the privileged one exported/external as well. This way, the default (ie non-webapk) ChildProcessCreationParams applies to all connections * Never dynamically query whether a service is exported or not, ie remove ChildProcessConnection.isExportedService. Every embedder should know itself whether its declared services are exported or not upfront and set the correct ChildProcessCreationParams Also I don't think I fully understand the bug description. Utility process is sandboxed, so should match renderer process. So I'm not entirely sure why the original description grouped utility and gpu process together. Maybe someone can explain that, or maybe description is so stale it no longer matters. Only guess I have is original description only applies to webapk, but not to webview or monochrome, in which case crbug.com/664530 should be the correct fix. fwiw the reason behind not treating sandboxed vs privileged differently is soon, privilelged won't automatically imply gpu process anymore. there's some interest in having unsandboxed utility process as well. And also reduce cognitive load when someone reads this code again Anyone see issues or have objections?
,
May 11 2017
Oh crbug.com/663888 is my second bullet point I think. And sounds like that could break webapk service if that's done before crbug.com/664530 is fixed. But since we are not shipping with webapk service, is it ok for me to just not care about that for now? Btw is there a tracking bug to enable webapk service? because all the bugs mentioned here should be blocking that.
,
May 11 2017
hmm, I was conflating external with exported I think. So these configurations are possible I think: * exported=false, external=false (regular chrome), do not need BIND_EXTERNAL_SERVICE * exported=true, external=true (monochrome, webvew), need BIND_EXTERNAL_SERVICE * exported=true, external=false (webapk in both regular chrome and monochrome), do not need BIND_EXTERNAL_SERVICE Looking at that.. why do we even need ChildProcessConnection.isExportedService right now? Seems external==true implies exported==true already, and BIND_EXTERNAL_SERVICE is entirely determined by external state..? Also since this is easy to generate from list above.. The manual test configuration for any changes in this area is: * regular chrome * webapk in regular chrome * monochrome * webapk in monochrome * webview in monochrome * standalone webview
,
May 12 2017
no one responded, so I guess everyone is in violent agreement with me :p So how do I enable webapk in a dev build now? I followed the instructions, enable "unknown resources", and added --enable-improved-a2hs. But add to home screen menu on plus.google.com is still just a shortcut
,
May 12 2017
Yes, I think that's the right approach. I vaguely remember proposing this some time in the past on a bug/CL but can't recall what the objections (if any) were, and can't find it to check.
,
May 12 2017
hanxi helped me setting up webapk local build Turns out in webapk, initializeChildProcessCreationParams is called very late in start up, after warm up connection, and after native WebContentsImpl is created. So even if I modify mCanLaunchRendererInWebApkProcess to true, it doesn't actually start services in webapk package. Oh so broken..
,
May 12 2017
Issue 663888 has been merged into this issue.
,
May 12 2017
Hmm, even after working around all of that and successfully creating the webapk service, got these errors: [ERROR:child_thread_impl.cc(730)] Request for unknown Channel-associated interface: chrome::mojom::RendererConfiguration But at least the service started, so good enough for this testing.
,
May 12 2017
oh.. so only isolated process can be bound externally. So the runtime detection of whether a service is exported is actually to make sure gpu process don't get the BIND_EXTERNAL_SERVICE flag. That's fine for now.. but does that mean webview can't ever have unsandboxed child services?
,
May 12 2017
Yes, that's correct. You can only bind an isolated service as external, because that makes it much simpler: isolated services already run as a different UID than the app which owns them, and they don't have access to any application data files. So, associating them with a different application than the one which defines them is basically just an accounting trick with no security implications. The worst thing an isolated service can do to you if you bind to it with EXTERNAL_SERVICE is cause your app to show up as a high user of battery/memory in the system stats. Allowing a non-isolated service to be bound as external would have to run that service *as the client's UID*, giving it full access to the client's data. In fact, the normal Android model here would imply running the service in the client's actual same process unless specified otherwise. You'd have to be *really* sure that the other app is trustworthy before you bound to it in this way. So, the feature is restricted in this way to prevent creating big security risks.
,
May 12 2017
yeah makes sense, but throws a wrench in the unsandboxed utility process plan. I guess will need a --in-process-utility switch or something like that, if one doesn't already exist. +jcivelli: you removed sandboxed state from connection, but I think this requires it to be added back, in order to remove isExportedService, which is a runtime detection so much worse
,
May 12 2017
That RendererConfiguration error only occurs with monochrome, but not regular chrome. How odd..
,
May 12 2017
Looking at the chunk of code that this bug was originally filed for, it's pretty clear that it's also for distinguishing privileged vs sandboxed services. So ChildProcessCreationParams, both getPackageName and getIsExternalService should only apply to sandboxed service.
,
May 15 2017
I was OOO on Friday but I think your breakdown makes sense AFAIK. #8 = :( #15: Correct - currently it was only planning to launch renderers this way. There's the IWebApkApi which runs in a privileged process in the WebApk but we hadn't yet planned to run any of chrome's privileged process in the WebApk. I could see this being desirable in the limit of servicification/mojo/mandoline but isn't yet. I had tested it at one point (launching a privileged process via mojo in the webapk) and it worked but we don't really need it yet
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89547e5d3101e03824714b5808f63c312321b526 commit 89547e5d3101e03824714b5808f63c312321b526 Author: Bo Liu <boliu@chromium.org> Date: Tue May 23 22:16:46 2017 android: Remove isExportedService and related changes See bug for more detailed discussion. The isExportedService and changing package check were added specifically so that privileged process are not affected by some settings in ChildProcessCreationParams. So rename those members to be explicitly about sandboxed services only, and use them as such. And then remove these run-time checks. Manually tested these configurations on Android N device: * regular chrome * webapk in regular chrome * monochrome * webapk in monochrome * webview in monochrome * standalone webview BUG= 620102 Change-Id: Idfce8f7e8cafc167eb6585032431db1373e9b827 Reviewed-on: https://chromium-review.googlesource.com/506469 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Commit-Queue: Bo Liu <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#474081} [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/base/android/java/src/org/chromium/base/process_launcher/ChildProcessCreationParams.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/java/src/org/chromium/content/browser/SpareChildConnection.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/89547e5d3101e03824714b5808f63c312321b526/content/public/android/junit/src/org/chromium/content/browser/SpareChildConnectionTest.java
,
May 23 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by siev...@chromium.org
, Jun 17 2016