Package Crashpad's handler into various APKs [chromium/src : master]
Joshua Peraza (Gerrit)
agrieve for build/android/gyp/apkbuilder.py and chrome/android/*
boliu for android_webview/test/BUILD.gn and content/shell/android/BUILD.gn
halliwell for chromecast/BUILD.gn
Thanks!
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #6 to this change.
Package Crashpad's handler into various APKs
The Crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not be
ignored by Android's APK installer.
This CL starts packaging the handler executable into apks for content
shell, android webview, chrome, and chromecast.
Bug: crashpad:30
Change-Id: If5b3752f26455e5c7aef3278b4bd2076ef1b7b65
---
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
M chrome/android/chrome_public_apk_tmpl.gni
M chrome/android/static_initializers.gni
M chromecast/BUILD.gn
M content/shell/android/BUILD.gn
6 files changed, 21 insertions(+), 4 deletions(-)
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Luke Halliwell (Gerrit)
Patch set 6:Code-Review +1
Bo (Gerrit)
2 comments:
Patch Set #6, Line 11:
ignored by Android's APK installer.agrieve is master of this area to decide if there's anything better, but this feels so hacky.. :(
File android_webview/test/BUILD.gn:
Patch Set #6, Line 60:
loadable_modules = [ "$root_out_dir/libcrashpad_handler.so" ]multiple questions here:
doing this in each apk seems really brittle. Is there a way to propagate this through dependencies? eg apks that depend on components:crash (or whatever) implicitly gets this?
is $root_out_dir shared for for multiarch builds? say if the apk contains both 32 bit and 64 bit libraries, do we need to bundle this twice? does that work now? You can try building system_webview_apk with arm64 to try that config
and finally, this is only the webview test apk. if the dependency thing doesn't work out, then you should be adding this in android_webview/system_webview_apk_tmpl.gni instead
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
agrieve (Gerrit)
I think adding at loadable_modules in multiple spots is fine, but I do have two other concerns:
1) Monochrome.apk sets android:extractNativeLibs="false" in its manifest, so no libraries are extracted in this case. Will this break for this case?
2) The .so files is 200kb. This is quite large! Can it be made smaller?
Joshua Peraza (Gerrit)
Patch Set 6:
I think adding at loadable_modules in multiple spots is fine, but I do have two other concerns:
1) Monochrome.apk sets android:extractNativeLibs="false" in its manifest, so no libraries are extracted in this case. Will this break for this case?
Yeah, that sounds like it'll be a problem. :( As-is, the APK fails to install with an error about libcrashpad_handler.so being compressed in the APK. I'm now investigating putting the handler into assets or resources, but it looks like files placed there also aren't expected to be accessed directly? DIR_ASSETS in base_paths_android.cc is unimplemented, but maybe that's just to encourage accessing assets through AssetManager? The end-goal is to be able to execv the file from a signal handler so it'd be best if it was already extracted somewhere accessible.
2) The .so files is 200kb. This is quite large! Can it be made smaller?
As a start, it shares some code also being linked into libchrome/libmonochrome/libwebview, so maybe I can try pulling that out into it's own .so. We should also be able to stop linking breakpad after this is enabled, though I'm not sure how much that'll save us. I'll keep investigating!
Bo: I think the answers to your questions mostly depend on the resolution to 1).
Bo (Gerrit)
Patch Set 6:
Patch Set 6:
I think adding at loadable_modules in multiple spots is fine, but I do have two other concerns:
1) Monochrome.apk sets android:extractNativeLibs="false" in its manifest, so no libraries are extracted in this case. Will this break for this case?
Yeah, that sounds like it'll be a problem. :( As-is, the APK fails to install with an error about libcrashpad_handler.so being compressed in the APK. I'm now investigating putting the handler into assets or resources, but it looks like files placed there also aren't expected to be accessed directly? DIR_ASSETS in base_paths_android.cc is unimplemented, but maybe that's just to encourage accessing assets through AssetManager? The end-goal is to be able to execv the file from a signal handler so it'd be best if it was already extracted somewhere accessible.
2) The .so files is 200kb. This is quite large! Can it be made smaller?
As a start, it shares some code also being linked into libchrome/libmonochrome/libwebview, so maybe I can try pulling that out into it's own .so. We should also be able to stop linking breakpad after this is enabled, though I'm not sure how much that'll save us. I'll keep investigating!
Bo: I think the answers to your questions mostly depend on the resolution to 1).
I'm still not clear on what should happen and what does happen for an apk that contains both 32bit and 64bit code?
Joshua Peraza (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
I think adding at loadable_modules in multiple spots is fine, but I do have two other concerns:
1) Monochrome.apk sets android:extractNativeLibs="false" in its manifest, so no libraries are extracted in this case. Will this break for this case?
Yeah, that sounds like it'll be a problem. :( As-is, the APK fails to install with an error about libcrashpad_handler.so being compressed in the APK. I'm now investigating putting the handler into assets or resources, but it looks like files placed there also aren't expected to be accessed directly? DIR_ASSETS in base_paths_android.cc is unimplemented, but maybe that's just to encourage accessing assets through AssetManager? The end-goal is to be able to execv the file from a signal handler so it'd be best if it was already extracted somewhere accessible.
2) The .so files is 200kb. This is quite large! Can it be made smaller?
As a start, it shares some code also being linked into libchrome/libmonochrome/libwebview, so maybe I can try pulling that out into it's own .so. We should also be able to stop linking breakpad after this is enabled, though I'm not sure how much that'll save us. I'll keep investigating!
Bo: I think the answers to your questions mostly depend on the resolution to 1).
I'm still not clear on what should happen and what does happen for an apk that contains both 32bit and 64bit code?
I was able to get the libcrashpad_handler.so installed for both 32-bit and 64-bit by adding it to secondary_abi_loadable_modules here:
https://cs.chromium.org/chromium/src/android_webview/system_webview_apk_tmpl.gni?rcl=5dc35e997fc12e292128ae2e9af77f0355e9fcbc&l=32
I expect I'd need to do that for any other APKs that might have multiple ABIs as well.
I think having just the 32-bit version installed for an apk that has both 32-bit and 64-bit code *could* be fine. The crashpad handler has generally been written to be able to trace cross-bitness but that feature hasn't been tested yet and there's at least one TODO that would prevent a 64-bit ARM handler from tracing a 32-bit process.
agrieve (Gerrit)
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
Joshua Peraza (Gerrit)
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).
We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
Joshua Peraza (Gerrit)
Patch Set 6:
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).
We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
Actually, if extractNativeLibs=false is only N+, memfd_create() might be an option there.
agrieve (Gerrit)
Patch Set 6:
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
Ha! That's clever!
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
extractNativeLibs=false is indeed N+.
+torne in case he has thoughts on any of this.
Joshua Peraza (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
Ha! That's clever!
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
extractNativeLibs=false is indeed N+.
+torne in case he has thoughts on any of this.
Some more thoughts:
Trying to call into AssetManager::open* at crash time would probably be pretty risky, so we might need to always have the file mapped, which means more memory vs storage.
crashpad_handler also links libbase.cr.so so we'd need to statically link crashpad_handler or otherwise arrange for libbase to also be extracted.
Richard Coles (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
Ha! That's clever!
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
extractNativeLibs=false is indeed N+.
+torne in case he has thoughts on any of this.
The mapping between android versions and kernel versions is extremely weak, and unless you've found an official source that says that N+ *definitely* has a new enough kernel version you may be out of luck there (also: are these features always enabled? because if they're kernel config options and android doesn't require them then depending on them still may not work). In general kernel features are only expected to work if there's CTS test coverage.
Richard Coles (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
AFAIK, no other files get extracted upon installation. All assets and resources get accessed via memory mapping the .apk.
Does Android support fexecve? And would that api work for a memory-mapped file? Wondering if you could execute it without extracting.
There's no fexecve, but it just calls execve on /proc/self/fd/<fd> so we can do that ourselves if we can get a file descriptor set up.
Ha! That's clever!
However, it looks like the file descriptor returned by AssetManager::open* would be for the whole apk, which I think wouldn't work with fexecve/execve(/proc/self/fd<fd>).We could map it ourselves with memfd_create (Linux 3.17+) or O_TMPFILE (Linux 3.11+) but I don't think those are old enough, so we might need to end up practically extracting it anyways.
extractNativeLibs=false is indeed N+.
+torne in case he has thoughts on any of this.
Some more thoughts:
Trying to call into AssetManager::open* at crash time would probably be pretty risky, so we might need to always have the file mapped, which means more memory vs storage.
crashpad_handler also links libbase.cr.so so we'd need to statically link crashpad_handler or otherwise arrange for libbase to also be extracted.
Aren't you going to have to statically link crashpad_handler anyway? libbase.cr.so doesn't exist in a shipping build, there is only one library, and you can't/shouldn't change that: the WebView library loading mechanism has a bug that prevents it from being able to correctly load more than one library, and there's an ongoing discussion about how the component build is not intended to be of shippable quality and does not guarantee a lack of ODR violations..
Richard Coles (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
I think adding at loadable_modules in multiple spots is fine, but I do have two other concerns:
1) Monochrome.apk sets android:extractNativeLibs="false" in its manifest, so no libraries are extracted in this case. Will this break for this case?
Yeah, that sounds like it'll be a problem. :( As-is, the APK fails to install with an error about libcrashpad_handler.so being compressed in the APK. I'm now investigating putting the handler into assets or resources, but it looks like files placed there also aren't expected to be accessed directly? DIR_ASSETS in base_paths_android.cc is unimplemented, but maybe that's just to encourage accessing assets through AssetManager? The end-goal is to be able to execv the file from a signal handler so it'd be best if it was already extracted somewhere accessible.
2) The .so files is 200kb. This is quite large! Can it be made smaller?
As a start, it shares some code also being linked into libchrome/libmonochrome/libwebview, so maybe I can try pulling that out into it's own .so. We should also be able to stop linking breakpad after this is enabled, though I'm not sure how much that'll save us. I'll keep investigating!
Bo: I think the answers to your questions mostly depend on the resolution to 1).
I'm still not clear on what should happen and what does happen for an apk that contains both 32bit and 64bit code?
I was able to get the libcrashpad_handler.so installed for both 32-bit and 64-bit by adding it to secondary_abi_loadable_modules here:
https://cs.chromium.org/chromium/src/android_webview/system_webview_apk_tmpl.gni?rcl=5dc35e997fc12e292128ae2e9af77f0355e9fcbc&l=32
I expect I'd need to do that for any other APKs that might have multiple ABIs as well.I think having just the 32-bit version installed for an apk that has both 32-bit and 64-bit code *could* be fine. The crashpad handler has generally been written to be able to trace cross-bitness but that feature hasn't been tested yet and there's at least one TODO that would prevent a 64-bit ARM handler from tracing a 32-bit process.
Just having the 32-bit version won't work for 64-bit-only platforms, which are coming in future.
Joshua Peraza (Gerrit)
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
Option 1 seems like the most promising short-term solution to me, assuming it's secure, with 3 being a possible long-term solution.
Richard Coles (Gerrit)
Patch Set 6:
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
I don't think this can work from renderer processes, either for Chrome or for WebView: the isolated_app sandbox cannot access application data directories at all. Even if this worked, this would be extremely undesirable for WebView, as every app would have to do it separately, which would waste a load of disk space on redundant copies of this file.
We have historically gone to great lengths to ensure that webview doesn't require any files to be extracted in this way.
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
This is also highly undesirable for webview due to the memory usage - we don't want every webview app to have to copy this to ram, and it's not possible to share ashmem segments across processes in different trust domains safely.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
This won't work on older OS versions where bundles aren't shipped to the device due to lack of support, and the APKs are all merged together by the Play Store serving infrastructure.
Joshua Peraza (Gerrit)
Patch Set 6:
Patch Set 6:
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
I don't think this can work from renderer processes, either for Chrome or for WebView: the isolated_app sandbox cannot access application data directories at all. Even if this worked, this would be extremely undesirable for WebView, as every app would have to do it separately, which would waste a load of disk space on redundant copies of this file.
Renderers won't need access to it. The browser launches the handler either for itself or on behalf of a child process, but I don't have a solution for the redundant copies. :(
We have historically gone to great lengths to ensure that webview doesn't require any files to be extracted in this way.
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
This is also highly undesirable for webview due to the memory usage - we don't want every webview app to have to copy this to ram, and it's not possible to share ashmem segments across processes in different trust domains safely.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
This won't work on older OS versions where bundles aren't shipped to the device due to lack of support, and the APKs are all merged together by the Play Store serving infrastructure.
It'll be inconvenient (if possible?) to deal with different packaging strategies, but I think it only needs to work for Monochrome/N+ unless webview doesn't extract native libraries on older versions too?
Richard Coles (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
I don't think this can work from renderer processes, either for Chrome or for WebView: the isolated_app sandbox cannot access application data directories at all. Even if this worked, this would be extremely undesirable for WebView, as every app would have to do it separately, which would waste a load of disk space on redundant copies of this file.
Renderers won't need access to it. The browser launches the handler either for itself or on behalf of a child process, but I don't have a solution for the redundant copies. :(
We have historically gone to great lengths to ensure that webview doesn't require any files to be extracted in this way.
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
This is also highly undesirable for webview due to the memory usage - we don't want every webview app to have to copy this to ram, and it's not possible to share ashmem segments across processes in different trust domains safely.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
This won't work on older OS versions where bundles aren't shipped to the device due to lack of support, and the APKs are all merged together by the Play Store serving infrastructure.
It'll be inconvenient (if possible?) to deal with different packaging strategies, but I think it only needs to work for Monochrome/N+ unless webview doesn't extract native libraries on older versions too?
Standalone webview currently extracts its native libraries, to avoid having to have a different version for L vs M+ (L doesn't support loading from APK natively). So, yeah, we may only need to solve this for Monochrome. I don't know if setting extractNativeLibs differently in different splits is even expected to work, however - it's anyone's guess whether this will do the expected thing.
Joshua Peraza (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
I don't think this can work from renderer processes, either for Chrome or for WebView: the isolated_app sandbox cannot access application data directories at all. Even if this worked, this would be extremely undesirable for WebView, as every app would have to do it separately, which would waste a load of disk space on redundant copies of this file.
Renderers won't need access to it. The browser launches the handler either for itself or on behalf of a child process, but I don't have a solution for the redundant copies. :(
We have historically gone to great lengths to ensure that webview doesn't require any files to be extracted in this way.
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
This is also highly undesirable for webview due to the memory usage - we don't want every webview app to have to copy this to ram, and it's not possible to share ashmem segments across processes in different trust domains safely.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
This won't work on older OS versions where bundles aren't shipped to the device due to lack of support, and the APKs are all merged together by the Play Store serving infrastructure.
It'll be inconvenient (if possible?) to deal with different packaging strategies, but I think it only needs to work for Monochrome/N+ unless webview doesn't extract native libraries on older versions too?
Standalone webview currently extracts its native libraries, to avoid having to have a different version for L vs M+ (L doesn't support loading from APK natively). So, yeah, we may only need to solve this for Monochrome. I don't know if setting extractNativeLibs differently in different splits is even expected to work, however - it's anyone's guess whether this will do the expected thing.
I've got a rough proof-of-concept that seems to work for Chrome by calling execv on /system/bin/app_process{32} and has it run a java class CrashpadHandlerMain, which loads the native library and calls into crashpad::HandlerMain(). I'm thinking of linking all the Crashpad code into libchrome/libwebview and using LibraryLoader. Do you see any reason that's not going to work for WebView?
Richard Coles (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Some options:
1) Package crashpad_handler as an asset and extract it to DIR_ANDROID_APP_DATA at crash_reporter initialization. This at least works on an emulator, but I'm not sure whether SELinux might sometimes not allow an exec here. There looks to be some precedent in chrome installing executable code here with webapk extracting its runtime in app_dex. Also for webview, is it safe to be executing code from a directory writable by the embedding application?
I don't think this can work from renderer processes, either for Chrome or for WebView: the isolated_app sandbox cannot access application data directories at all. Even if this worked, this would be extremely undesirable for WebView, as every app would have to do it separately, which would waste a load of disk space on redundant copies of this file.
Renderers won't need access to it. The browser launches the handler either for itself or on behalf of a child process, but I don't have a solution for the redundant copies. :(
We have historically gone to great lengths to ensure that webview doesn't require any files to be extracted in this way.
2) If we want to go the memory-mapping from the APK route, I think we can use ashmem instead of memfd_create. Downsides are mapping it at every initialization, more memory and a file descriptor used, and possibly closing/losing the file descriptor.
This is also highly undesirable for webview due to the memory usage - we don't want every webview app to have to copy this to ram, and it's not possible to share ashmem segments across processes in different trust domains safely.
3) I started investigating putting crashpad_handler into its own android_app_bundle_module where it would set extractNativeLibs="true", but it looks like support for bundles is still in progress. https://bugs.chromium.org/p/chromium/issues/detail?id=820459
This won't work on older OS versions where bundles aren't shipped to the device due to lack of support, and the APKs are all merged together by the Play Store serving infrastructure.
It'll be inconvenient (if possible?) to deal with different packaging strategies, but I think it only needs to work for Monochrome/N+ unless webview doesn't extract native libraries on older versions too?
Standalone webview currently extracts its native libraries, to avoid having to have a different version for L vs M+ (L doesn't support loading from APK natively). So, yeah, we may only need to solve this for Monochrome. I don't know if setting extractNativeLibs differently in different splits is even expected to work, however - it's anyone's guess whether this will do the expected thing.
I've got a rough proof-of-concept that seems to work for Chrome by calling execv on /system/bin/app_process{32} and has it run a java class CrashpadHandlerMain, which loads the native library and calls into crashpad::HandlerMain(). I'm thinking of linking all the Crashpad code into libchrome/libwebview and using LibraryLoader. Do you see any reason that's not going to work for WebView?
I can't think of any reason that won't work for WebView specifically, but I would expect Android to be generally against this and arguing that app_process is not intended to be a public or stable API :|
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #12 to this change.
Package Crashpad's handler into various APKs
For Chromecast, Content Shell, and non-monochrome Clank/Webview:
The Crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not be
ignored by Android's APK installer.
For Monochrome:
The Crashpad handler is linked into libmonochrome.so. At crash-time
/system/bin/app_process{32,64} is exec-ed, running CrashpadMain.java
which loads libmonochrome.so and runs the native HandlerMain().
This strategy is not used for non-monochrome APKs (i.e. pre-N) because
on L, Android's loader is not yet capable of loading native libraries
from the APK. This is normally performed by the Chromium linker which
can't be used by CrashpadMain.java because /system/bin/app_process
doesn't initialize the process like a normal Android application
(specifically, no ContextImpl has been created, so any calls to e.g.
appContext.getApplicationInfo() will segfault).
Bug: crashpad:30
Change-Id: If5b3752f26455e5c7aef3278b4bd2076ef1b7b65
---
M android_webview/BUILD.gn
M android_webview/system_webview_apk_tmpl.gni
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
M chrome/android/BUILD.gn
M chrome/android/chrome_public_apk_tmpl.gni
M chromecast/BUILD.gn
M components/crash/android/BUILD.gn
M components/crash/android/DEPS
A components/crash/android/crashpad_main.cc
A components/crash/android/java/src/org/chromium/components/crash/browser/CrashpadMain.java
M components/crash/content/app/crashpad_linux.cc
M content/shell/android/BUILD.gn
13 files changed, 235 insertions(+), 30 deletions(-)
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
+mark for components/crash
This works the same as before for Chromecast, Content Shell, and Clank/System WebView.
Monochrome now uses /system/bin/app_process to launch the handler.
Richard Coles (Gerrit)
Monochrome now uses /system/bin/app_process to launch the handler.
Have you talked to someone in Android about whether this is okay?
Joshua Peraza (Gerrit)
Patch Set 12:
Monochrome now uses /system/bin/app_process to launch the handler.
Have you talked to someone in Android about whether this is okay?
Narayan says /system/bin/app_process will likely break at some point. /system/bin/dalvikvm is less likely to break, but is also unsupported. If we need to that can easily be swapped in in Crashpad.
That said, I think app_process will work for now and we can transition to executing the linker if/when that is option. If we reach a point where neither of those are an option, we can fall back to ashmem mapping an executable and execv-ing the file descriptor.
Mark Mentovai (Gerrit)
Let’s try this for now. Gotta start somewhere.
Patch set 12:Code-Review +1
Richard Coles (Gerrit)
Yeah I don't think there is another alternative that's more likely to work :(
Is there any mechanism for us to actually catch when this isn't working in the field, though, other than a dropoff of crash reports? I'm concerned that this might just not work on some specific device or exact OS build and we'll fail to notice the lack of reports for that specific configuration due to it being too small a fraction of our overall data.
Joshua Peraza (Gerrit)
Patch Set 12:
Yeah I don't think there is another alternative that's more likely to work :(
Is there any mechanism for us to actually catch when this isn't working in the field, though, other than a dropoff of crash reports? I'm concerned that this might just not work on some specific device or exact OS build and we'll fail to notice the lack of reports for that specific configuration due to it being too small a fraction of our overall data.
Errors will currently be logged here:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/posix/double_fork_and_exec.cc?rcl=efc1b61ec1db1a01cc7aa29474c51ed9daf86b3b&l=121
I think these won't make it back to us right now without a report to attach to, but it seems very do-able, to at least record metrics when this fails to launch the handler for child process crashes.
We can also extend the ChildProcessCrashObserver callback to upload logcats without minidumps:
https://chromium-review.googlesource.com/c/chromium/src/+/989401/82/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
Richard Coles (Gerrit)
Patch Set 12:
Patch Set 12:
Yeah I don't think there is another alternative that's more likely to work :(
Is there any mechanism for us to actually catch when this isn't working in the field, though, other than a dropoff of crash reports? I'm concerned that this might just not work on some specific device or exact OS build and we'll fail to notice the lack of reports for that specific configuration due to it being too small a fraction of our overall data.
Errors will currently be logged here:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/posix/double_fork_and_exec.cc?rcl=efc1b61ec1db1a01cc7aa29474c51ed9daf86b3b&l=121I think these won't make it back to us right now without a report to attach to, but it seems very do-able, to at least record metrics when this fails to launch the handler for child process crashes.
Yeah, I think we should try to record metrics for the new failure points like this one.
We can also extend the ChildProcessCrashObserver callback to upload logcats without minidumps:
https://chromium-review.googlesource.com/c/chromium/src/+/989401/82/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
That would also be good - we don't upload logcat in webview but it seems like any failure here should affect chrome and webview both, so we should be able to see it via chrome's data.
The CL itself looks OK.
Patch set 12:Code-Review +1
agrieve (Gerrit)
Basically lgtm /w some nits. But looks like this increases monochrome size by 100kb, and pre-monochrome by significantly more (the bot doesn't measure pre-monochome):
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/55960
If you haven't already done so, could you look through the list of symbols (from the supersize link in the bot) to see if they all make sense (need to exist), and gut-check their sizes?
Could you also mention on the commit description what the binary size increase is for monochrome & chrome_modern (so that binary size sheriffs will know). Might also be worth commenting on how much size we'll get back when breakpad is removed.
It looks like breakpad is ~34kb, which puts crashpad at 3x the size for monochrome. Does this seem right?
4 comments:
File chrome/android/chrome_public_apk_tmpl.gni:
Patch Set #12, Line 203:
is_monochrome = trueset this within monochrome_public_common_apk_or_module_tmpl to make it clear that that's what uses it.
File components/crash/android/java/src/org/chromium/components/crash/browser/CrashpadMain.java:
Patch Set #12, Line 7:
import android.support.annotation.Keep;Looks like this works only because we depend on play services:
https://cs.chromium.org/chromium/src/out/android-Debug/gen/third_party/android_deps/com_google_android_gms_play_services_basement_java/proguard.txtThis is a bit fragile, so you should either copy those proguard rules into:
https://cs.chromium.org/chromium/src/base/android/proguard/chromium_code.flagsOr use @UsedByReflection instead.
Patch Set #12, Line 21:
throw new RuntimeException("Cannot load native library", e);nit: Remove the string here. It doesn't add any context.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Exceptions
File components/crash/content/app/crashpad_linux.cc:
Patch Set #12, Line 215:
bool BuildEnvironmentWithAPK(std::vector<std::string>* result) {nit: I think style guide says to do "WithApk" rather than "WithAPK"
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
agrieve (Gerrit)
One other thing I thought of - should we add an enable_crashpad GN arg in case canary testing shows that we need to turn it off (so we don't need to revert the commit)?
Joshua Peraza (Gerrit)
Updates to the commit message about binary size are still in progress. I think I'd do that with tools/binary_size/diagnose_bloat.py --target chrome_modern_public_apk <...>?
The symbols in the supersize diff generally look correct. I do see one feature I could disable since it doesn't really provide much use on Android yet and could save a few KB. I think a not-insignificant portion of the difference between Breakpad and Crashpad is that Crashpad can trace cross-bitness, so lots of code paths are duplicated (via templates). e.g. The top two largest Crashpad symbols in that diff are for ElfImageReader and ExceptionSnapshots, both very bitness dependent.
Some more TODOs that are probably better left to later:
For J, K: We could link the handler directly into libchrome.so and let that be our handler executable. This would de-dup portions of libbase that are currently linked into both libchrome.so and libcrashpad_handler.so.
For L, M: It might be possible to get the chromium linker (or Bionic's linker on M) to dlopen libchrome.so from the APK for a trampoline libcrashpad_handler.so to de-dup libbase.
Patch Set 14:
One other thing I thought of - should we add an enable_crashpad GN arg in case canary testing shows that we need to turn it off (so we don't need to revert the commit)?
I've been thinking about this, but not sure about whether it'd be worth the extra effort to put everything affected by this transition behind build flags. (The enabling CL: https://chromium-review.googlesource.com/c/chromium/src/+/989401/84) Is the ability to roll-back not the greatest build flag of all? :D
4 comments:
Patch Set #12, Line 203:
is_monochrome = true
set this within monochrome_public_common_apk_or_module_tmpl to make it clear that that's what uses i […]
This is monochrome_public_common_apk_or_module_tmpl.
File components/crash/android/java/src/org/chromium/components/crash/browser/CrashpadMain.java:
Patch Set #12, Line 7:
import org.chromium.base.annotations.JNINamespace;Looks like this works only because we depend on play services: […]
Done
nit: Remove the string here. It doesn't add any context. […]
Done
File components/crash/content/app/crashpad_linux.cc:
Patch Set #12, Line 215:
bool BuildEnvironmentWithApk(std::vector<std::string>* result) {
nit: I think style guide says to do "WithApk" rather than "WithAPK"
Done
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Richard Coles (Gerrit)
Patch Set 15:
For L, M: It might be possible to get the chromium linker (or Bionic's linker on M) to dlopen libchrome.so from the APK for a trampoline libcrashpad_handler.so to de-dup libbase.
FYI, we don't use Bionic's linker on M any more either - we always use the chromium linker for non-monochrome builds.
agrieve (Gerrit)
For non-monochrome, that command looks right:
tools/binary_size/diagnose_bloat.py --target chrome_modern_public_apk
For monochrome, you can use that command as well, or look at the binary size trybot link (click "show experimental" to see it).
Nice observation about the cross-bitness! I suppose that's probably required for webview though?
Using the chromium linker for pre-N *should* work fine as well, but does add in another point of failure in case crashes are coming from the linker... Maybe the binary size savings / consistency with monochrome would be worth it though?
1 comment:
Patch Set #12, Line 203:
is_monochrome = true
This is monochrome_public_common_apk_or_module_tmpl.
Sorry, should be chrome_public_common_apk_or_module_tmpl. aka, the block starting on line 211
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #18 to this change.
Package Crashpad's handler into various APKs
For Chromecast, Content Shell, and non-monochrome Clank/Webview:
The Crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not be
ignored by Android's APK installer.
For Monochrome:
The Crashpad handler is linked into libmonochrome.so. At crash-time
/system/bin/app_process{32,64} is exec-ed, running CrashpadMain.java
which loads libmonochrome.so and runs the native HandlerMain().
This strategy is not used for non-monochrome APKs (i.e. pre-N) because
on L, Android's loader is not yet capable of loading native libraries
from the APK. This is normally performed by the Chromium linker which
can't be used by CrashpadMain.java because /system/bin/app_process
doesn't initialize the process like a normal Android application
(specifically, no ContextImpl has been created, so any calls to e.g.
appContext.getApplicationInfo() will segfault).
Binary-Size: MonochromePublic.apk increases in size by 100 KB.
ChromeModernPublic.apk increases in size by 203KB (587 KB
increase in install size because libcrashpad_handler.so is extracted
from the APK). Possible mitigations for this increase are TODO:
For J, K: We could link the handler directly into libchrome.so and let
that be our handler executable. This would de-dup portions of libbase
that are currently linked into both libchrome.so and
libcrashpad_handler.so.
For L, M: It might be possible to get the chromium linker (or Bionic's
linker on M) to dlopen libchrome.so from the APK for a trampoline
libcrashpad_handler.so to de-dup libbase.
Bug: crashpad:30
Change-Id: If5b3752f26455e5c7aef3278b4bd2076ef1b7b65
---
M android_webview/BUILD.gn
M android_webview/system_webview_apk_tmpl.gni
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
M chrome/android/BUILD.gn
M chrome/android/chrome_public_apk_tmpl.gni
M chromecast/BUILD.gn
M components/crash/android/BUILD.gn
M components/crash/android/DEPS
A components/crash/android/crashpad_main.cc
A components/crash/android/java/src/org/chromium/components/crash/browser/CrashpadMain.java
M components/crash/content/app/crashpad_linux.cc
M content/shell/android/BUILD.gn
13 files changed, 238 insertions(+), 30 deletions(-)
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
File chrome/android/chrome_public_apk_tmpl.gni:
Patch Set #12, Line 203:
template("monochrome_public_common_apk_or_module_tmpl") {Sorry, should be chrome_public_common_apk_or_module_tmpl. […]
Ahh, that makes more sense. Thanks!
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
> Nice observation about the cross-bitness! I suppose that's probably required for webview though?
>
It'll let us install a handler for a single bitness in the future, or do things like let the handler ride-along in some system process. For it's first go, we're starting with just same-bitness tracing.
Using the chromium linker for pre-N *should* work fine as well, but does add in another point of failure in case crashes are coming from the linker... Maybe the binary size savings / consistency with monochrome would be worth it though?
Good point!
agrieve (Gerrit)
Thanks for adding the binary size numbers (+benmason fyi)
Certainly any work to reduce would be appreciated, but I agree that we need to get this going.
If we aren't going to enable crashpad for m71 (branch in 2 weeks), then we should disable the packaging for it.
Patch set 18:Code-Review +1
Bo (Gerrit)
content stamp
Patch set 19:Code-Review +1
Joshua Peraza (Gerrit)
Patch Set 18: Code-Review+1
Thanks for adding the binary size numbers (+benmason fyi)
Certainly any work to reduce would be appreciated, but I agree that we need to get this going.
If we aren't going to enable crashpad for m71 (branch in 2 weeks), then we should disable the packaging for it.
Agreed!
Patch set 19:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Reviewed-on: https://chromium-review.googlesource.com/1150774
Reviewed-by: Bo <bo...@chromium.org>
Reviewed-by: agrieve <agr...@chromium.org>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Richard Coles <to...@chromium.org>
Reviewed-by: Luke Halliwell <hall...@chromium.org>
Commit-Queue: Joshua Peraza <jpe...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594853}
---
M android_webview/BUILD.gn
M android_webview/system_webview_apk_tmpl.gni
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
M chrome/android/BUILD.gn
M chrome/android/chrome_public_apk_tmpl.gni
M chromecast/BUILD.gn
M components/crash/android/BUILD.gn
M components/crash/android/DEPS
A components/crash/android/crashpad_main.cc
A components/crash/android/java/src/org/chromium/components/crash/browser/CrashpadMain.java
M components/crash/content/app/crashpad_linux.cc
M content/shell/android/BUILD.gn
13 files changed, 238 insertions(+), 30 deletions(-)
Takuto Ikuta (Gerrit)
This change causes android builder failure?
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-marshmallow-arm64-rel/8579
Joshua Peraza (Gerrit)
Joshua Peraza has created a revert of this change.
To view, visit change 1150774. To unsubscribe, or for help writing mail filters, visit settings.
Shimi Zhang (Gerrit)
Patch Set 20:
This change causes android builder failure?
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-marshmallow-arm64-rel/8579
I believe so, I filed http://crbug.com/890075