Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2063213002:
Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. (Closed)
|
Created:
4 years, 6 months ago by Xi Han Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, michaelbai, mimee, Theresa, sgurun-gerrit only, kravula_chromium.org, amineer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error.
The crash is introduced in CL:
https://codereview.chromium.org/2017963003/
The crash happens when initializing a utility process with
ChildProcessCreationParams is null. The extra binding flag is only added
when this parameter isn't null.
To fix the crash, we update the ChildProcessLaunch#start() to make sure
always use pre-set ChildProcessCreationParams to create a child process.
BUG=619563
Committed: https://crrev.com/d83004386e6c5f4a924d424c2772f3a57c7e6d2b
Cr-Commit-Position: refs/heads/master@{#399821}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update comment. #
Total comments: 2
Patch Set 3 : Update comments #Patch Set 4 : Add a TODO. #Patch Set 5 : Fire bug 620102. #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ========== to ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ==========
hanxi@chromium.org changed reviewers: + michaelbai@chromium.org
hanxi@chromium.org changed reviewers: - michaelbai@chromium.org
hanxi@chromium.org changed reviewers: + mariakhomenko@chromium.org
Hi Maria, could you please take a look? Thanks!
lgtm after you fix my suggestion https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // package name. Ugh, this is not very clean, but I don't have a good suggestion for you. Let's just expand this comment a bit, I guess. "Except renderer process, all other child processes should use Chrome's package name. In WebApk, ChildProcessCreationParams are initialized with WebApk's package name. Make a copy of the WebApk's params, but replace the package with Chrome's package to use when initializing a non-renderer processes."
hanxi@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: I need a OWNER review for content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java. Could you please take a look? Thanks! https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // package name. On 2016/06/14 17:04:49, Maria wrote: > Ugh, this is not very clean, but I don't have a good suggestion for you. > > Let's just expand this comment a bit, I guess. > > "Except renderer process, all other child processes should use Chrome's package > name. In WebApk, ChildProcessCreationParams are initialized with WebApk's > package name. Make a copy of the WebApk's params, but replace the package with > Chrome's package to use when initializing a non-renderer processes." This sounds better, thanks!
michaelbai@chromium.org changed reviewers: + michaelbai@chromium.org
https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // package name. On 2016/06/14 17:17:51, Xi Han wrote: > On 2016/06/14 17:04:49, Maria wrote: > > Ugh, this is not very clean, but I don't have a good suggestion for you. > > > > Let's just expand this comment a bit, I guess. > > > > "Except renderer process, all other child processes should use Chrome's > package > > name. In WebApk, ChildProcessCreationParams are initialized with WebApk's > > package name. Make a copy of the WebApk's params, but replace the package with > > Chrome's package to use when initializing a non-renderer processes." > > This sounds better, thanks! The comment isn't true, the WebView's child process needs its own package name, can we only replace the package name if it is not set? i.e context.getPackageName() is the default package name.
This is a bit messy and unclear what the contract here really is, which I realized when I was trying to suggest a test or assertion: So how would you formulate that? Is it that in ChildServiceConnection(bindFlags, needsExtraBindFlags) if |needsExtraBindFlags| is |true| then |mCreationParams| must not be null? How all of that propagates from the embedder and through the classes is also hacky. Needless to say that the content layer here should not know anything about WebAPKs or whatever, but it seems like it has some logic hardcoded to that. https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // name. In WebAPK, ChildProcessCreationParams are initialized with WebAPK's > Except renderer process, all other child processes should use Chrome's package name. Can you also explain why here in the comment (both why renderers are different and what the package name matters for here anyways)?
https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // package name. On 2016/06/14 17:46:21, michaelbai wrote: > The comment isn't true, the WebView's child process needs its own package name, > can we only replace the package name if it is not set? i.e > context.getPackageName() is the default package name. > Does WebView use different package name for GPU or utility process? For WebAPK, think of the following case: WebAPK is launching when Chrome isn't running, so the ChildProcessCreationParams is set as WebAPK's package name. Since GPU process needs to be initialized at the same time, it ends up using WebAPK's package name, which is wrong.
On 2016/06/14 17:55:18, sievers wrote: > This is a bit messy and unclear what the contract here really is, which I > realized when I was trying to suggest a test or assertion: So how would you > formulate that? I will add some WebAPK's instrument tests in ChildProcessLauncherTest. > Is it that in ChildServiceConnection(bindFlags, needsExtraBindFlags) if > |needsExtraBindFlags| is |true| then |mCreationParams| must not be null? There is no such assumption, and I did see |needsExtraBindFlags| is |true| while |mCreationParams| is null when debugging, which causes the crash. When the new Android sdk is used, we could simple add Context.BIND_EXTERNAL_SERVICE in here, no matter whether the |mCreationParams| is null or not. However, this flag is hidden in clank and we cannot add it directly now. > How all of that propagates from the embedder and through the classes is also > hacky. Needless to say that the content layer here should not know anything > about WebAPKs or whatever, but it seems like it has some logic hardcoded to > that. I agree. After using the new sdk, I think we could revert this fix and keep using params is null in ChildProcessLauncher#start(). Any opinion? I can fire a bug to trace it if we all agree that is the right thing to do. https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // package name. On 2016/06/14 17:46:21, michaelbai wrote: > On 2016/06/14 17:17:51, Xi Han wrote: > > On 2016/06/14 17:04:49, Maria wrote: > > > Ugh, this is not very clean, but I don't have a good suggestion for you. > > > > > > Let's just expand this comment a bit, I guess. > > > > > > "Except renderer process, all other child processes should use Chrome's > > package > > > name. In WebApk, ChildProcessCreationParams are initialized with WebApk's > > > package name. Make a copy of the WebApk's params, but replace the package > with > > > Chrome's package to use when initializing a non-renderer processes." > > > > This sounds better, thanks! > > The comment isn't true, the WebView's child process needs its own package name, > can we only replace the package name if it is not set? i.e > context.getPackageName() is the default package name. > My understanding is WebView only have renderer process with own package name, not other child process, like GPU or utility process. Right? https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: // name. In WebAPK, ChildProcessCreationParams are initialized with WebAPK's On 2016/06/14 17:55:18, sievers wrote: > > Except renderer process, all other child processes should use Chrome's package > name. > > Can you also explain why here in the comment (both why renderers are different > and what the package name matters for here anyways)? Expand the comment here, and let me know if you have any suggestion to make the comment better.
On 2016/06/14 18:35:43, Xi Han wrote: > On 2016/06/14 17:55:18, sievers wrote: > > This is a bit messy and unclear what the contract here really is, which I > > realized when I was trying to suggest a test or assertion: So how would you > > formulate that? > > I will add some WebAPK's instrument tests in ChildProcessLauncherTest. > > > Is it that in ChildServiceConnection(bindFlags, needsExtraBindFlags) if > > |needsExtraBindFlags| is |true| then |mCreationParams| must not be null? > > There is no such assumption, and I did see |needsExtraBindFlags| is |true| while > |mCreationParams| is null when debugging, which causes the crash. > When the new Android sdk is used, we could simple add > Context.BIND_EXTERNAL_SERVICE in here, no matter whether the |mCreationParams| > is null or not. However, this flag is hidden in clank and we cannot add it > directly now. > > > How all of that propagates from the embedder and through the classes is also > > hacky. Needless to say that the content layer here should not know anything > > about WebAPKs or whatever, but it seems like it has some logic hardcoded to > > that. > > I agree. After using the new sdk, I think we could revert this fix and keep > using params is null in ChildProcessLauncher#start(). Any opinion? I can fire a > bug to trace it if we all agree that is the right thing to do. > I think whatever is clear and documented (and maybe tested) at the API-level is fine. To be honest it is not obvious from the current code from looking at ChildProcessCreationParams that they only get added for renderers. It seems like this was added as a hack for WebView initially. Won't we still need the params to let the application override the package name or how would we find out if we need to apply the external_service flag? The subtle implications of passing the package name are also confusing. So for the renderer for example, it will figure out getNumberOfServices() from the application manifest while for utility and gpu it comes from the chrome manifest, right? > https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > (right): > > https://codereview.chromium.org/2063213002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: > // package name. > On 2016/06/14 17:46:21, michaelbai wrote: > > On 2016/06/14 17:17:51, Xi Han wrote: > > > On 2016/06/14 17:04:49, Maria wrote: > > > > Ugh, this is not very clean, but I don't have a good suggestion for you. > > > > > > > > Let's just expand this comment a bit, I guess. > > > > > > > > "Except renderer process, all other child processes should use Chrome's > > > package > > > > name. In WebApk, ChildProcessCreationParams are initialized with WebApk's > > > > package name. Make a copy of the WebApk's params, but replace the package > > with > > > > Chrome's package to use when initializing a non-renderer processes." > > > > > > This sounds better, thanks! > > > > The comment isn't true, the WebView's child process needs its own package > name, > > can we only replace the package name if it is not set? i.e > > context.getPackageName() is the default package name. > > > > My understanding is WebView only have renderer process with own package name, > not other child process, like GPU or utility process. Right? > > https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > (right): > > https://codereview.chromium.org/2063213002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:664: > // name. In WebAPK, ChildProcessCreationParams are initialized with WebAPK's > On 2016/06/14 17:55:18, sievers wrote: > > > Except renderer process, all other child processes should use Chrome's > package > > name. > > > > Can you also explain why here in the comment (both why renderers are different > > and what the package name matters for here anyways)? > > Expand the comment here, and let me know if you have any suggestion to make the > comment better.
I have a bug to remove BIND_EXTERNAL_SERVICE once we could use the N sdk in upstream, after that, we will not set ChildProcessCreationParams in Monchrome, then only WebView and WebAPKs need it for the package name reason.
Description was changed from ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ========== to ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash is introduced in CL: https://codereview.chromium.org/2017963003/ The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ==========
I add a TODO to keep track. PTAL, thanks. On 2016/06/14 19:12:49, sievers wrote: > I think whatever is clear and documented (and maybe tested) at the API-level is > fine. > To be honest it is not obvious from the current code from looking at > ChildProcessCreationParams that they only get added for renderers. > It seems like this was added as a hack for WebView initially. Yes, updated the CL description and added my previoious CL which might explain better. > Won't we still need the params to let the application override the package name > or how would we find out if we need to apply the external_service flag? As long as the service is exported and on sdk N+. See (https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...). > The subtle implications of passing the package name are also confusing. So for > the renderer for example, it will figure out getNumberOfServices() from the > application manifest while for utility and gpu it comes from the chrome > manifest, right? > Yes, the getNumberOfServices() would treat renderer and other child process differently.
lgtm but can you also file a bug to remove the embedder implementation details from the content-layer here? Plus clean up and better define the behavior of the overrides we allow here (for bind flags and package name and how it interacts with 'number of services' and how they are defined etc.).
On 2016/06/14 21:05:58, sievers wrote: > lgtm but can you also file a bug to remove the embedder implementation details > from the content-layer here? > Plus clean up and better define the behavior of the overrides we allow here (for > bind flags and package name and how it interacts with 'number of services' and > how they are defined etc.). Fire crbug.com/620102. Thanks for the suggestions.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2063213002/#ps80001 (title: "Fire bug 620102.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063213002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063213002/80001
Message was sent while issue was closed.
Description was changed from ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash is introduced in CL: https://codereview.chromium.org/2017963003/ The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ========== to ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash is introduced in CL: https://codereview.chromium.org/2017963003/ The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash is introduced in CL: https://codereview.chromium.org/2017963003/ The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 ========== to ========== Fix Chrome crashes on launch due to BIND_EXTERNAL_SERVICE error. The crash is introduced in CL: https://codereview.chromium.org/2017963003/ The crash happens when initializing a utility process with ChildProcessCreationParams is null. The extra binding flag is only added when this parameter isn't null. To fix the crash, we update the ChildProcessLaunch#start() to make sure always use pre-set ChildProcessCreationParams to create a child process. BUG=619563 Committed: https://crrev.com/d83004386e6c5f4a924d424c2772f3a57c7e6d2b Cr-Commit-Position: refs/heads/master@{#399821} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d83004386e6c5f4a924d424c2772f3a57c7e6d2b Cr-Commit-Position: refs/heads/master@{#399821} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
