Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487673006:
Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. (Closed)
|
Created:
4 years, 1 month ago by Xi Han Modified:
4 years, 1 month ago CC:
chromium-reviews, dominickn+watch_chromium.org, jam, pkotwicz+watch_chromium.org, agrieve+watch_chromium.org, darin-cc_chromium.org, zpeng+watch_chromium.org, android-webview-reviews_chromium.org, michaelbai, pkotwicz Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK.
In this CL:
1. Add a isExternalService flag to ChildProcessCreationParams
2. Set this flag in the Monochrome and WebApkActivity.
This CL depends on:
https://codereview.chromium.org/2491943003/
https://codereview.chromium.org/2494683004/
https://chrome-internal-review.googlesource.com/#/c/304255/
BUG=663888, 663898
Committed: https://crrev.com/f4d8bfbe302b3f8cd12f2d445712205a875f63a1
Cr-Commit-Position: refs/heads/master@{#431597}
Patch Set 1 #
Total comments: 8
Patch Set 2 : boliu@'s comments. #Patch Set 3 : Add a TODO. #Messages
Total messages: 30 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. BUG=663888, 663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. This CL depends on: https://codereview.chromium.org/2491943003/ https://codereview.chromium.org/2494683004/ https://chrome-internal-review.googlesource.com/#/c/304255/ BUG=663888, 663898 ==========
hanxi@chromium.org changed reviewers: + boliu@chromium.org
Hi Bo, could you please take a look? Thanks!
hanxi@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: isExternalService, LibraryProcessType.PROCESS_CHILD); so is WebApkActivity running inside the chrome browser process? So for monochrome + webapk, we want isExteranlService = true for monochrome's renderer process, but false for webapk, and we need to switch dynamically at run time? Am I understanding that correctly? not that this is worse than before, but doesn't appear setting a global var is a super safe thing to make this work.. also appears this never sets isExternalService back to the original value when !isForWebApk https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:239: Context.BIND_AUTO_CREATE); extraBindFlags required here as well? https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:339: return new ChildProcessCreationParams(packageName, false, false /* isExternalService */
PTAL, thanks! https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: isExternalService, LibraryProcessType.PROCESS_CHILD); On 2016/11/10 22:14:51, boliu wrote: > so is WebApkActivity running inside the chrome browser process? So for > monochrome + webapk, we want isExteranlService = true for monochrome's renderer > process, but false for webapk, and we need to switch dynamically at run time? Am > I understanding that correctly? > > not that this is worse than before, but doesn't appear setting a global var is a > super safe thing to make this work.. > > also appears this never sets isExternalService back to the original value when > !isForWebApk We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK is no longer foreground. Otherwise, Chrome will continue to use a WebAPK's params as long as a WebAPK render is launched. That means, all of the renderer process created after a WebAPK's render process will use WebAPK's params. You can see we pass in "false" for |isForWebApk| when a WebApk is no longer in foreground, and it will just update the global var to Chrome's default params. Also, we have to put the initializeChildProcessCreationParams(true) in onResume(). This is because the WebAPK's creation params is needed to create a new render process when user bring WebAPK back from recents but the renderer process has gone. https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:239: Context.BIND_AUTO_CREATE); On 2016/11/10 22:14:51, boliu wrote: > extraBindFlags required here as well? Good catch, thanks! https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2487673006/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:339: return new ChildProcessCreationParams(packageName, false, On 2016/11/10 22:14:51, boliu wrote: > false /* isExternalService */ Done.
https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: isExternalService, LibraryProcessType.PROCESS_CHILD); On 2016/11/10 22:42:14, Xi Han wrote: > On 2016/11/10 22:14:51, boliu wrote: > > so is WebApkActivity running inside the chrome browser process? So for > > monochrome + webapk, we want isExteranlService = true for monochrome's > renderer > > process, but false for webapk, and we need to switch dynamically at run time? > Am > > I understanding that correctly? > > > > not that this is worse than before, but doesn't appear setting a global var is > a > > super safe thing to make this work.. > > > > also appears this never sets isExternalService back to the original value when > > !isForWebApk > > We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK is no > longer foreground. Otherwise, Chrome will continue to use a WebAPK's params as > long as a WebAPK render is launched. That means, all of the renderer process > created after a WebAPK's render process will use WebAPK's params. You can see we > pass in "false" for |isForWebApk| when a WebApk is no longer in foreground, and > it will just update the global var to Chrome's default params. Oops, I wasn't reading carefully. I thought it was ChildProcessCreationParams.get at the top, rather than chrome.getChildProcessCreationParams. > Also, we have to put the initializeChildProcessCreationParams(true) in > onResume(). This is because the WebAPK's creation params is needed to create a > new render process when user bring WebAPK back from recents but the renderer > process has gone. Are all chrome renderer shutdown when WebApkActivity is active. And are all webapk renderers shutdown when webapkactivity is paused? What happens if chrome's code is tries to start a chrome renderer process while the webapk activity is active?
https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: isExternalService, LibraryProcessType.PROCESS_CHILD); On 2016/11/10 23:10:34, boliu wrote: > On 2016/11/10 22:42:14, Xi Han wrote: > > On 2016/11/10 22:14:51, boliu wrote: > > > so is WebApkActivity running inside the chrome browser process? So for > > > monochrome + webapk, we want isExteranlService = true for monochrome's > > renderer > > > process, but false for webapk, and we need to switch dynamically at run > time? > > Am > > > I understanding that correctly? > > > > > > not that this is worse than before, but doesn't appear setting a global var > is > > a > > > super safe thing to make this work.. > > > > > > also appears this never sets isExternalService back to the original value > when > > > !isForWebApk > > > > We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK is no > > longer foreground. Otherwise, Chrome will continue to use a WebAPK's params as > > long as a WebAPK render is launched. That means, all of the renderer process > > created after a WebAPK's render process will use WebAPK's params. You can see > we > > pass in "false" for |isForWebApk| when a WebApk is no longer in foreground, > and > > it will just update the global var to Chrome's default params. > > Oops, I wasn't reading carefully. I thought it was > ChildProcessCreationParams.get at the top, rather than > chrome.getChildProcessCreationParams. > > > Also, we have to put the initializeChildProcessCreationParams(true) in > > onResume(). This is because the WebAPK's creation params is needed to create a > > new render process when user bring WebAPK back from recents but the renderer > > process has gone. > > > Are all chrome renderer shutdown when WebApkActivity is active. And are all > webapk renderers shutdown when webapkactivity is paused? I don't think chrome renderer are shutdown when WebAPK is active, but their corresponding ChromeTabbedActivity are paused. WebApk renderers are just in background when webapkactivity is paused, but it is possible that the renderers are killed or crashed when webapkactivity is paused. > What happens if chrome's code is tries to start a chrome renderer process while > the webapk activity is active? I don't think chrome can start a chrome render process while the webapk activity is active. From my understanding, there is only one activity would be active on Androoid, other activities are just paused. Once WebApkActivity is active, other Chrome's activities are paused and have to become active if they want to start a chrome renderer process.
On 2016/11/11 00:16:58, Xi Han wrote: > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > (right): > > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: > isExternalService, LibraryProcessType.PROCESS_CHILD); > On 2016/11/10 23:10:34, boliu wrote: > > On 2016/11/10 22:42:14, Xi Han wrote: > > > On 2016/11/10 22:14:51, boliu wrote: > > > > so is WebApkActivity running inside the chrome browser process? So for > > > > monochrome + webapk, we want isExteranlService = true for monochrome's > > > renderer > > > > process, but false for webapk, and we need to switch dynamically at run > > time? > > > Am > > > > I understanding that correctly? > > > > > > > > not that this is worse than before, but doesn't appear setting a global > var > > is > > > a > > > > super safe thing to make this work.. > > > > > > > > also appears this never sets isExternalService back to the original value > > when > > > > !isForWebApk > > > > > > We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK is > no > > > longer foreground. Otherwise, Chrome will continue to use a WebAPK's params > as > > > long as a WebAPK render is launched. That means, all of the renderer process > > > created after a WebAPK's render process will use WebAPK's params. You can > see > > we > > > pass in "false" for |isForWebApk| when a WebApk is no longer in foreground, > > and > > > it will just update the global var to Chrome's default params. > > > > Oops, I wasn't reading carefully. I thought it was > > ChildProcessCreationParams.get at the top, rather than > > chrome.getChildProcessCreationParams. > > > > > Also, we have to put the initializeChildProcessCreationParams(true) in > > > onResume(). This is because the WebAPK's creation params is needed to create > a > > > new render process when user bring WebAPK back from recents but the renderer > > > process has gone. > > > > > > Are all chrome renderer shutdown when WebApkActivity is active. And are all > > webapk renderers shutdown when webapkactivity is paused? > > I don't think chrome renderer are shutdown when WebAPK is active, but their > corresponding ChromeTabbedActivity are paused. WebApk renderers are just in > background when webapkactivity is paused, but it is possible that the renderers > are killed or crashed when webapkactivity is paused. > > > What happens if chrome's code is tries to start a chrome renderer process > while > > the webapk activity is active? > > I don't think chrome can start a chrome render process while the webapk activity > is active. From my understanding, there is only one activity would be active on > Androoid, other activities are just paused. Once WebApkActivity is active, other > Chrome's activities are paused and have to become active if they want to start a > chrome renderer process. That's a lot of assumptions, and anyone can accidentally break them. There is nothing preventing code to start a renderer while the activity is in the background. Eg something could be warming up a renderer while chrome is in the background etc. I don't think this code is safe to ship.. A proper way to do this would be to make the param per activity maybe
On 2016/11/11 00:23:00, boliu wrote: > On 2016/11/11 00:16:58, Xi Han wrote: > > > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > (right): > > > > > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: > > isExternalService, LibraryProcessType.PROCESS_CHILD); > > On 2016/11/10 23:10:34, boliu wrote: > > > On 2016/11/10 22:42:14, Xi Han wrote: > > > > On 2016/11/10 22:14:51, boliu wrote: > > > > > so is WebApkActivity running inside the chrome browser process? So for > > > > > monochrome + webapk, we want isExteranlService = true for monochrome's > > > > renderer > > > > > process, but false for webapk, and we need to switch dynamically at run > > > time? > > > > Am > > > > > I understanding that correctly? > > > > > > > > > > not that this is worse than before, but doesn't appear setting a global > > var > > > is > > > > a > > > > > super safe thing to make this work.. > > > > > > > > > > also appears this never sets isExternalService back to the original > value > > > when > > > > > !isForWebApk > > > > > > > > We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK is > > no > > > > longer foreground. Otherwise, Chrome will continue to use a WebAPK's > params > > as > > > > long as a WebAPK render is launched. That means, all of the renderer > process > > > > created after a WebAPK's render process will use WebAPK's params. You can > > see > > > we > > > > pass in "false" for |isForWebApk| when a WebApk is no longer in > foreground, > > > and > > > > it will just update the global var to Chrome's default params. > > > > > > Oops, I wasn't reading carefully. I thought it was > > > ChildProcessCreationParams.get at the top, rather than > > > chrome.getChildProcessCreationParams. > > > > > > > Also, we have to put the initializeChildProcessCreationParams(true) in > > > > onResume(). This is because the WebAPK's creation params is needed to > create > > a > > > > new render process when user bring WebAPK back from recents but the > renderer > > > > process has gone. > > > > > > > > > Are all chrome renderer shutdown when WebApkActivity is active. And are all > > > webapk renderers shutdown when webapkactivity is paused? > > > > I don't think chrome renderer are shutdown when WebAPK is active, but their > > corresponding ChromeTabbedActivity are paused. WebApk renderers are just in > > background when webapkactivity is paused, but it is possible that the > renderers > > are killed or crashed when webapkactivity is paused. > > > > > What happens if chrome's code is tries to start a chrome renderer process > > while > > > the webapk activity is active? > > > > I don't think chrome can start a chrome render process while the webapk > activity > > is active. From my understanding, there is only one activity would be active > on > > Androoid, other activities are just paused. Once WebApkActivity is active, > other > > Chrome's activities are paused and have to become active if they want to start > a > > chrome renderer process. > > That's a lot of assumptions, and anyone can accidentally break them. There is > nothing preventing code to start a renderer while the activity is in the > background. Eg something could be warming up a renderer while chrome is in the > background etc. I don't think this code is safe to ship.. > > A proper way to do this would be to make the param per activity maybe Do you suggest that every ChromeActivity has a initlizeChildProcessCreationParams() in onResume()? We could set chrome's default creation parameter for other activities except WebApkActivities. But will the params work for webvew?
On 2016/11/11 00:51:08, Xi Han wrote: > On 2016/11/11 00:23:00, boliu wrote: > > On 2016/11/11 00:16:58, Xi Han wrote: > > > > > > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2487673006/diff/40001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:167: > > > isExternalService, LibraryProcessType.PROCESS_CHILD); > > > On 2016/11/10 23:10:34, boliu wrote: > > > > On 2016/11/10 22:42:14, Xi Han wrote: > > > > > On 2016/11/10 22:14:51, boliu wrote: > > > > > > so is WebApkActivity running inside the chrome browser process? So for > > > > > > monochrome + webapk, we want isExteranlService = true for monochrome's > > > > > renderer > > > > > > process, but false for webapk, and we need to switch dynamically at > run > > > > time? > > > > > Am > > > > > > I understanding that correctly? > > > > > > > > > > > > not that this is worse than before, but doesn't appear setting a > global > > > var > > > > is > > > > > a > > > > > > super safe thing to make this work.. > > > > > > > > > > > > also appears this never sets isExternalService back to the original > > value > > > > when > > > > > > !isForWebApk > > > > > > > > > > We need to clean up the WebAPK's ChildProcessCreationParams when WebAPK > is > > > no > > > > > longer foreground. Otherwise, Chrome will continue to use a WebAPK's > > params > > > as > > > > > long as a WebAPK render is launched. That means, all of the renderer > > process > > > > > created after a WebAPK's render process will use WebAPK's params. You > can > > > see > > > > we > > > > > pass in "false" for |isForWebApk| when a WebApk is no longer in > > foreground, > > > > and > > > > > it will just update the global var to Chrome's default params. > > > > > > > > Oops, I wasn't reading carefully. I thought it was > > > > ChildProcessCreationParams.get at the top, rather than > > > > chrome.getChildProcessCreationParams. > > > > > > > > > Also, we have to put the initializeChildProcessCreationParams(true) in > > > > > onResume(). This is because the WebAPK's creation params is needed to > > create > > > a > > > > > new render process when user bring WebAPK back from recents but the > > renderer > > > > > process has gone. > > > > > > > > > > > > Are all chrome renderer shutdown when WebApkActivity is active. And are > all > > > > webapk renderers shutdown when webapkactivity is paused? > > > > > > I don't think chrome renderer are shutdown when WebAPK is active, but their > > > corresponding ChromeTabbedActivity are paused. WebApk renderers are just in > > > background when webapkactivity is paused, but it is possible that the > > renderers > > > are killed or crashed when webapkactivity is paused. > > > > > > > What happens if chrome's code is tries to start a chrome renderer process > > > while > > > > the webapk activity is active? > > > > > > I don't think chrome can start a chrome render process while the webapk > > activity > > > is active. From my understanding, there is only one activity would be active > > on > > > Androoid, other activities are just paused. Once WebApkActivity is active, > > other > > > Chrome's activities are paused and have to become active if they want to > start > > a > > > chrome renderer process. > > > > That's a lot of assumptions, and anyone can accidentally break them. There is > > nothing preventing code to start a renderer while the activity is in the > > background. Eg something could be warming up a renderer while chrome is in the > > background etc. I don't think this code is safe to ship.. > > > > A proper way to do this would be to make the param per activity maybe > > Do you suggest that every ChromeActivity has a > initlizeChildProcessCreationParams() in onResume()? We could set chrome's > default creation parameter for other activities except WebApkActivities. But > will the params work for webvew? No. The bindings code in content needs to be aware of there are different params for different activities, and that a particular binding belongs needs to use this param. Basically the param being a global will never work, it needs to be "per activity", or per something else that's at least as fine grained as activities. agrieve probably knows the bindings code better than I do. Maybe he has an opinion here?
anyway, this CL doesn't make existing code any worse, so lgtm but I think this issue probably needs to be a ship blocker for webapk
Yeah, agreed with bo: LGTM as this doesn't make anything worse and lets us reland the non-webapk related parts without breaking webapk, but I don't think WebAPK should ship in this state. The child process creation parameters need to be passed down for each child process, not just be a global. I thought they were already passed down :/
File a bug to get a better solution. Thanks for the suggestions. I need to figure out how to let ChildProcessLauncher know the current activity.
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, I need OWNERS review for: -chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java Could you please take a look? Thanks!
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rs lgtm
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2487673006/#ps80001 (title: "Add a TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. This CL depends on: https://codereview.chromium.org/2491943003/ https://codereview.chromium.org/2494683004/ https://chrome-internal-review.googlesource.com/#/c/304255/ BUG=663888, 663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. This CL depends on: https://codereview.chromium.org/2491943003/ https://codereview.chromium.org/2494683004/ https://chrome-internal-review.googlesource.com/#/c/304255/ BUG=663888, 663898 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. This CL depends on: https://codereview.chromium.org/2491943003/ https://codereview.chromium.org/2494683004/ https://chrome-internal-review.googlesource.com/#/c/304255/ BUG=663888, 663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. In this CL: 1. Add a isExternalService flag to ChildProcessCreationParams 2. Set this flag in the Monochrome and WebApkActivity. This CL depends on: https://codereview.chromium.org/2491943003/ https://codereview.chromium.org/2494683004/ https://chrome-internal-review.googlesource.com/#/c/304255/ BUG=663888, 663898 Committed: https://crrev.com/f4d8bfbe302b3f8cd12f2d445712205a875f63a1 Cr-Commit-Position: refs/heads/master@{#431597} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f4d8bfbe302b3f8cd12f2d445712205a875f63a1 Cr-Commit-Position: refs/heads/master@{#431597} |
