Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488963004:
Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. (Closed)
DescriptionFix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK.
The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported
to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes
a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service
WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be
bound with the BIND_EXTERNAL_SERVICE flag.
This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService()
we check whether the service is an external service, rather than whether it is an
exported service. Only if it is an external service, we turn on the flag.
BUG=663898
Committed: https://crrev.com/70041c05e3ea17a1e67d0b2b1afb3929faf06e0d
Cr-Commit-Position: refs/heads/master@{#431251}
Patch Set 1 #
Total comments: 6
Patch Set 2 : boliu@'s comments. #
Total comments: 2
Patch Set 3 : Nits. #Messages
Total messages: 29 (17 generated)
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check externalService V.S. exported. Only when both externalService = true and exported = true, will turn on the flag. BUG=663898 ==========
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check externalService V.S. exported. Only when both externalService = true and exported = true, will turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check externalService V.S. exported. Only when both externalService = true and exported = true, will turn on the flag. BUG=663898 ==========
hanxi@chromium.org changed reviewers: + agrieve@chromium.org, boliu@chromium.org
Hi Andrew and Bo, could you please take a look? Thanks!
https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:253: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { don't change behavior for < N?
https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:253: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2016/11/09 21:51:42, boliu wrote: > don't change behavior for < N? The BIND_EXTERNAL_SERVICE is added on API 24 (https://developer.android.com/reference/android/content/Context.html#BIND_EXT...), so this flag doesn't be added for < N. This is my understanding, please correct me if I am wrong.
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...
https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:252: // TODO(hanxi): crbug.com/663888. probably deserves a comment why this fix is bad, and not a long term fix https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:253: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2016/11/09 22:04:00, Xi Han wrote: > On 2016/11/09 21:51:42, boliu wrote: > > don't change behavior for < N? > > The BIND_EXTERNAL_SERVICE is added on API 24 > (https://developer.android.com/reference/android/content/Context.html#BIND_EXT...), > so this flag doesn't be added for < N. This is my understanding, please correct > me if I am wrong. I meant previous code checked for exported regardless of android version. But since the method is renamed to External from Exported, perhaps it shouldn't be checking exported at all?
PTAL, thanks! https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:252: // TODO(hanxi): crbug.com/663888. On 2016/11/09 22:11:17, boliu wrote: > probably deserves a comment why this fix is bad, and not a long term fix Done. https://codereview.chromium.org/2488963004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:253: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2016/11/09 22:11:17, boliu wrote: > On 2016/11/09 22:04:00, Xi Han wrote: > > On 2016/11/09 21:51:42, boliu wrote: > > > don't change behavior for < N? > > > > The BIND_EXTERNAL_SERVICE is added on API 24 > > > (https://developer.android.com/reference/android/content/Context.html#BIND_EXT...), > > so this flag doesn't be added for < N. This is my understanding, please > correct > > me if I am wrong. > > I meant previous code checked for exported regardless of android version. But > since the method is renamed to External from Exported, perhaps it shouldn't be > checking exported at all? Yes, I understood why you have this concern. Besides, remove the exported check.
lgtm https://codereview.chromium.org/2488963004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:255: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { perhaps should move this to the top of the method and just early out return false
Hi Andrew, could you please take a look? Thanks! https://codereview.chromium.org/2488963004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java (right): https://codereview.chromium.org/2488963004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:255: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2016/11/09 22:26:39, boliu wrote: > perhaps should move this to the top of the method and just early out return > false Good idea, 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...
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check externalService V.S. exported. Only when both externalService = true and exported = true, will turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an externalService, not whether is exported. Only if it is an external service, we turn on the flag. BUG=663898 ==========
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an externalService, not whether is exported. Only if it is an external service, we turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an externalService, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ==========
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an externalService, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an external service, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
torne@chromium.org changed reviewers: + torne@chromium.org
LGTM as a short-term fix if the webapk is currently required to be signed by the correct signature anyway.
On 2016/11/10 13:42:48, Torne wrote: > LGTM as a short-term fix if the webapk is currently required to be signed by the > correct signature anyway. Yes, WebAPKs are signed by our WebAPK server and Chrome verifies their signatures every time when they are launched.
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 Link to the patchset: https://codereview.chromium.org/2488963004/#ps40001 (title: "Nits.")
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. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an external service, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an external service, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an external service, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 ========== to ========== Fix bug: turns on BIND_EXTERNAL_SERVICE flag on Android N crashes WebAPK. The ChildProcessConnectionImpl#isExportedService() checked whether a service is exported to decide whether to bind the service with flag BIND_EXTERNAL_SERVICE. However, it causes a crash to launch WebAPK when Chrome isn't launched yet. WebAPK has an exported Service WebApkSandboxedProcessService but it is not an "External Service", so it shouldn't be bound with the BIND_EXTERNAL_SERVICE flag. This CL has a quick fix to this bug: in ChildProcessConnectionImpl#isExportedService() we check whether the service is an external service, rather than whether it is an exported service. Only if it is an external service, we turn on the flag. BUG=663898 Committed: https://crrev.com/70041c05e3ea17a1e67d0b2b1afb3929faf06e0d Cr-Commit-Position: refs/heads/master@{#431251} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/70041c05e3ea17a1e67d0b2b1afb3929faf06e0d Cr-Commit-Position: refs/heads/master@{#431251} |
