Chrome crashed on start-up when restarting MinidumpUploadService |
|||
Issue descriptionSince https://codereview.chromium.org/2441623002/ Chrome needs to set a Chrome-specific delegate at start-up for use in MinidumpUploadService. If this delegate is not set MinidumpUploadService will crash. It seems like crashing Chrome (the entire application) when MinidumpUploadService is running will cause MinidumpUploadService to crash when we start Chrome again. IIUC the system then restarts MinidumpUploadService instead of Chrome starting it - meaning that we won't set the delegate (which is done in DeferredStartupHandler) before MinidumpUploadService has been started. stack of the crash that crashed the Chrome browser process: 10-27 17:37:02.610 15766 15766 E AndroidRuntime: Process: com.google.android.apps.chrome, PID: 15766 10-27 17:37:02.610 15766 15766 E AndroidRuntime: java.lang.RuntimeException: Unable to resume activity {com.google.android.apps.chrome/org.chromium.chrome.browser.ChromeTabbedActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'boolean android.content.Context.bindService(android.content.Intent, android.content.ServiceConnection, int)' on a null object reference 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3400) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3440) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1510) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.os.Handler.dispatchMessage(Handler.java:102) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.os.Looper.loop(Looper.java:154) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.ActivityThread.main(ActivityThread.java:6077) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at java.lang.reflect.Method.invoke(Native Method) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'boolean android.content.Context.bindService(android.content.Intent, android.content.ServiceConnection, int)' on a null object reference 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.gsa.GSAServiceClient.connect(GSAServiceClient.java:102) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.ChromeActivity.onStartWithNative(ChromeActivity.java:39122) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.ChromeTabbedActivity.onStartWithNative(ChromeTabbedActivity.java:465) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.init.NativeInitializationController.startNowAndProcessPendingItems(NativeInitializationController.java:252) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.init.AsyncInitializationActivity.onStart(AsyncInitializationActivity.java:6179) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.ChromeActivity.onStart(ChromeActivity.java:857) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at org.chromium.chrome.browser.ChromeTabbedActivity.onStart(ChromeTabbedActivity.java:459) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1248) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.Activity.performStart(Activity.java:6681) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.Activity.performRestart(Activity.java:6755) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.Activity.performResume(Activity.java:6760) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3377) 10-27 17:37:02.610 15766 15766 E AndroidRuntime: ... 8 more Stack of the crash in MinidumpUploadService at startup 10-27 17:37:50.439 16060 16060 E AndroidRuntime: java.lang.RuntimeException: Unable to instantiate service org.chromium.chrome.browser.crash.MinidumpUploadService: java.lang.RuntimeException: The upload delegate must be set before use. 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.app.ActivityThread.handleCreateService(ActivityThread.java:3147) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.app.ActivityThread.-wrap5(ActivityThread.java) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1550) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.os.Handler.dispatchMessage(Handler.java:102) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.os.Looper.loop(Looper.java:154) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.app.ActivityThread.main(ActivityThread.java:6077) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at java.lang.reflect.Method.invoke(Native Method) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: Caused by: java.lang.RuntimeException: The upload delegate must be set before use. 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at org.chromium.chrome.browser.crash.MinidumpUploadService.getUploadDelegate(MinidumpUploadService.java:82) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at org.chromium.chrome.browser.crash.MinidumpUploadService.<init>(MinidumpUploadService.java:67) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at java.lang.Class.newInstance(Native Method) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: at android.app.ActivityThread.handleCreateService(ActivityThread.java:3144) 10-27 17:37:50.439 16060 16060 E AndroidRuntime: ... 8 more I didn't realize that the system might be restarting MinidumpUploadService :/ I'm not sure how we can fix this (and still use a delegate in the MinidumpUploadService). Any ideas?
,
Oct 27 2016
Hmm, that might work, how would it reach the ChromeApplication class?
,
Oct 27 2016
(the MinidumpUploadService won't live in chrome/) :)
,
Oct 28 2016
Hmm, I'm not really sure how Android restarts processes. Is there a way for a process to register data that should survive a restart? Can that data be ... a delegate class to instantiate? Sounds kinda hairy, but maybe Android has utilities to support this use case, since it's probably not entirely uncommon that a process expects to be able to preserve some running state.
,
Oct 28 2016
I talked a little bit with Torne about this, here are some notes: 1. You can solve this problem at compile-time (use a delegate in components/ and define its implementation in chrome/) in C++ by declaring a header-file in components/ and adding the implementation for that header-file in chrome/. This isn't possible in Java since it doesn't have separate compiler and linker stages. 2. In Java, the ChromeApplication.onCreate method will be called before the onCreate method of any Services in Chrome. So, we could technically call MinidumpUploadService.setUploadDelegate within ChromeApplication.onCreate, and move the getUploadService check from the MinidumpUploadService ctor to its onCreate method. However, ChromeApplication is a sub-class of Application (an Android class that you can declare in your manifest to be able to perform tasks at important life-cycle events - like onCreate) which WebView can't use. In this particular case this isn't a problem since MinidumpUploadService won't be used by WebView. But in general putting stuff in a sub-class of Application means WebView might not have a corresponding place to put it (and any code in Application-subclasses will not be called from WebView) - and because of that Torne would like to remove at least the sub-classes of Application living in layers shared with WebView (content/ and base/), and potentially then the ChromeApplication as well. 3. There is still the alternative of just limiting componentization of minidump-uploading to the files used by WebView: CrashFileManager and MinidumpUploadCallable :). Personally I'm leaning towards just componentizing the parts of minidump-uploading used by WebView since that action won't include sharing any Services across layers (which seems to be the main cause of head-aches here) - even if we solve this problem now it might be a pain to work with later if e.g. ChromeApplication changes or goes away. I will still happily fix the bugs/ugliness we have stumbled upon (e.g. crbug.com/659938 ) when componentizing the whole minidump-uploading:).
,
Oct 28 2016
To answer comment #4 I think what you are referring to is the data you can store in Activity.onSaveInstanceState - which is then passed to Activity.onCreate(Bundle savedState). There doesn't seem to be a similar approach for Services - and even if there would be the saved state would have to be parcelable - having (instances of) classes be parcelable can be a pain that I don't think we want to rely on - as soon as they hold an object that isn't parcelable the classes themselves won't be either.
,
Oct 28 2016
Yeah, makes sense. Thanks for researching that! I'm okay with not componentizing the Service if it's too hairy to do so.
,
Oct 28 2016
Alright, given comments #5 and 7 I'll just componentize the stuff WebView needs - I'll revert https://codereview.chromium.org/2441623002/ on Monday (it seems wrong to revert something on a Friday evening ;)) and then I'll just move CrashFileManager and MinidumpUploadCallable to the minidump-upload component.
,
Oct 28 2016
SGTM, thanks for looking into the options.
,
Oct 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/012becd22e310a14a4cc53db562592a5622bd222 commit 012becd22e310a14a4cc53db562592a5622bd222 Author: gsennton <gsennton@chromium.org> Date: Mon Oct 31 08:44:20 2016 Revert of Split MinidumpUploadService into core- and Chrome-implementation. (patchset #5 id:80001 of https://codereview.chromium.org/2441623002/ ) Reason for revert: Componentizing the MinidumpUploadService is a pain - and not strictly necessary - we have to set the MinidumpUploadDelegate before the service uses this delegate, but the service might be started by the system rather than by Chrome itself. See crbug.com/660075 Original issue's description: > Split MinidumpUploadService into core- and Chrome-implementation. > > To componentize MinidumpUploadService we split it into its core > implementation and an implementation dependent on Chrome. The > Chrome-dependent parts now live in ChromeMinidumpUploadDelegate. > > To inject this delegate into the MinidumpUploadService we have to create > a Chrome-specific version of the MinidumpUploadService (inheriting from > it) named ChromeMinidumpUploadService. > > BUG=652719 > > Committed: https://crrev.com/271ef66ac8bab41f55f18586a1cd2fd2970d4d79 > Cr-Commit-Position: refs/heads/master@{#427989} TBR=isherman@chromium.org,mariakhomenko@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=652719, 660075 Review-Url: https://codereview.chromium.org/2466443002 Cr-Commit-Position: refs/heads/master@{#428667} [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java [delete] https://crrev.com/5cc479a2c0d7ba8de9f08948be37d8b323625865/chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploadDelegate.java [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/java_sources.gni [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java [modify] https://crrev.com/012becd22e310a14a4cc53db562592a5622bd222/components/minidump_uploader/BUILD.gn [delete] https://crrev.com/5cc479a2c0d7ba8de9f08948be37d8b323625865/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/util/MinidumpUploadDelegate.java
,
Oct 31 2016
,
Oct 31 2016
Issue 660799 has been merged into this issue. |
|||
►
Sign in to add a comment |
|||
Comment 1 by mariakho...@chromium.org
, Oct 27 2016