Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833893002:
[Android WebView] Trigger minidump uploading on renderer crash. (Closed)
Description[Android WebView] Trigger minidump uploading on renderer crash.
In MultiProcess mode the WebView renderer process can crash without
bringing down the browser process.
Such a crash causes a minidump to be generated - with this CL that
minidump is handled (copied, later to be uploaded) quickly after the
renderer process crash instead of during the next WebView startup.
BUG=674145
Review-Url: https://codereview.chromium.org/2833893002
Cr-Commit-Position: refs/heads/master@{#473871}
Committed: https://chromium.googlesource.com/chromium/src/+/9c0a1e4d561f51b8aa0861a952b0612934f9bbe6
Patch Set 1 #
Total comments: 15
Patch Set 2 : Post copying-tasks to SERIAL_EXECUTOR instead of THREAD_POOL_EXECUTOR + add several comments. #
Total comments: 2
Patch Set 3 : Remove unused aw_browser_procces implementation (and rename the actual implementation). #Patch Set 4 : Rebase over Shimi's native/ -> browser/ CL. #
Total comments: 2
Patch Set 5 : Move aw_browser_process.h into native-component in browser/ #Patch Set 6 : Rebase #
Messages
Total messages: 25 (5 generated)
gsennton@chromium.org changed reviewers: + paulmiller@chromium.org, tobiasjs@chromium.org
ATM this code doesn't trigger anything because minidump generation for renderers is off (see crbug.com/713762) - I tested by working around that though. Toby: ptal :) Paul: FYI (since I'm moving the gms-core call from WebViewChromiumFactoryProvider a little bit). https://codereview.chromium.org/2833893002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:415: AwBrowserProcess.setWebViewPackageName(webViewPackageName); AwBrowserProcess.setWebViewPackageName must be called before a renderer crash triggers minidump uploading. Are we sure that this is the case? (I assume so since we are calling from startChromiumLocked, but just want to double-check). I should probably add a comment about this ;) https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:158: public static void setWebViewPackageName(String webViewPackageName) { Having this is a bit ugly... but we fetch the WebView package name from the glue - so we won't be able to fetch it when calling from native -> triggerMinidumpUploading() otherwise.
https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:89: TriggerMinidumpUploading(); Oooh, another question for Toby: is the minidump for the render process generated at this point, or could we have a race here where the minidump hasn't been generated when we try to copy it from AwBrowserProcess?
https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:158: public static void setWebViewPackageName(String webViewPackageName) { On 2017/04/20 20:32:46, gsennton wrote: > Having this is a bit ugly... but we fetch the WebView package name from the glue > - so we won't be able to fetch it when calling from native -> > triggerMinidumpUploading() otherwise. I was going to do something like this anyway. The metrics code needs to check the package name in order to determine channel. Better to put the ugliness in your change than mine! :) https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:174: * @param updateMetricsConsent whether to update the metrics-consent value to represent the This flag feels hacky. Why not have 2 queryMetricsSetting() calls, 1 in the original location for both crashes and metrics, and 1 here for just crashes?
https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:158: public static void setWebViewPackageName(String webViewPackageName) { On 2017/04/20 23:38:51, paulmiller wrote: > On 2017/04/20 20:32:46, gsennton wrote: > > Having this is a bit ugly... but we fetch the WebView package name from the > glue > > - so we won't be able to fetch it when calling from native -> > > triggerMinidumpUploading() otherwise. > > I was going to do something like this anyway. The metrics code needs to check > the package name in order to determine channel. Better to put the ugliness in > your change than mine! :) Haha, damn ;) https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:174: * @param updateMetricsConsent whether to update the metrics-consent value to represent the On 2017/04/20 23:38:51, paulmiller wrote: > This flag feels hacky. Why not have 2 queryMetricsSetting() calls, 1 in the > original location for both crashes and metrics, and 1 here for just crashes? We could do that, I just don't like duplicating the whole statement of if(enableMinidumpUploadingForTesting) ... and also the queryMetricsSetting-statement. It feels like we could easily miss updating one of the two locations in an upcoming fix. WDYT?
https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:89: TriggerMinidumpUploading(); On 2017/04/20 20:37:06, gsennton wrote: > Oooh, another question for Toby: is the minidump for the render process > generated at this point, or could we have a race here where the minidump hasn't > been generated when we try to copy it from AwBrowserProcess? breakpad::CrashDumpManager is registered prior to AwBrowserTerminator so that this can be guaranteed. It's a little bit hairy because OnChildExit is called in order for the two objects on the UI thread, and both post to the FILE thread, and AwBrowserTerminator then posts back to the UI thread. But the net result is that this will come after the minidump has been moved to the crash directory. https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:168: handleMinidumpsAndSetMetricsConsent(sWebViewPackageName, false /* updateMetricsConsent */); I think now we could get into a situation where we have more than one AsyncTask posted by handleMinidumps running concurrently.
https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:174: * @param updateMetricsConsent whether to update the metrics-consent value to represent the On 2017/04/21 15:46:33, gsennton wrote: > On 2017/04/20 23:38:51, paulmiller wrote: > > This flag feels hacky. Why not have 2 queryMetricsSetting() calls, 1 in the > > original location for both crashes and metrics, and 1 here for just crashes? > > We could do that, I just don't like duplicating the whole statement of > if(enableMinidumpUploadingForTesting) ... and also the > queryMetricsSetting-statement. It feels like we could easily miss updating one > of the two locations in an upcoming fix. WDYT? I don't think that calling queryMetricsSetting twice counts as duplicating code. That's sort of the point of functions, to call them from multiple places, to avoid duplicating code. But, up to you.
https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:168: handleMinidumpsAndSetMetricsConsent(sWebViewPackageName, false /* updateMetricsConsent */); On 2017/04/21 16:18:04, Tobias Sargeant wrote: > I think now we could get into a situation where we have more than one AsyncTask > posted by handleMinidumps running concurrently. Right, that might be undesirable ;) I think any alternative solution would make this code more complex (through synchronisation), though I guess even now we would have to argue about what might happen if two of these AsyncTasks interleave (which is a complex argument). I guess we could have a variable with three states on which to synchronize: states: Nothing_going_on Thread_copying_minidumps Thread_should_copy_minidumps_again code: AwBrowserProcess.handleMinidumps(): if (state == Nothing_going_on): post new AsyncTask set state = Thread_copying_minidumps else if (state == Thread_copying_minidumps): set state = Thread_should_copy_minidumps_again copyMinidumps() (in the AsyncTask): copy minidumps... before exiting: if (state == Thread_should_copy_minidumps_again): state = Thread_copying_minidumps rerun minidump-copying WDYT? (this logic is becoming complex enough that I would like some tests for it though... so would probably want to create some independent class for this logic itself, and then test that separately). https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:174: * @param updateMetricsConsent whether to update the metrics-consent value to represent the On 2017/04/21 17:50:01, paulmiller wrote: > On 2017/04/21 15:46:33, gsennton wrote: > > On 2017/04/20 23:38:51, paulmiller wrote: > > > This flag feels hacky. Why not have 2 queryMetricsSetting() calls, 1 in the > > > original location for both crashes and metrics, and 1 here for just crashes? > > > > We could do that, I just don't like duplicating the whole statement of > > if(enableMinidumpUploadingForTesting) ... and also the > > queryMetricsSetting-statement. It feels like we could easily miss updating one > > of the two locations in an upcoming fix. WDYT? > > I don't think that calling queryMetricsSetting twice counts as duplicating code. > That's sort of the point of functions, to call them from multiple places, to > avoid duplicating code. But, up to you. I'm more concerned about the whole enableMinidumpUploadingForTesting-logic: if (enableMinidumpUploadingForTesting) { handleMinidumps(); } queryMetricsSetting() -> { if (!enableMinidumpUploadingForTesting) { handleMinidumps(); } } it seems like duplicating that could cause divergence in the future :) (if this would only have been a call to queryMetricsSetting() I wouldn't mind duplicating that).
This should be fine now. PTAL :) https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:89: TriggerMinidumpUploading(); On 2017/04/21 16:18:04, Tobias Sargeant wrote: > On 2017/04/20 20:37:06, gsennton wrote: > > Oooh, another question for Toby: is the minidump for the render process > > generated at this point, or could we have a race here where the minidump > hasn't > > been generated when we try to copy it from AwBrowserProcess? > > breakpad::CrashDumpManager is registered prior to AwBrowserTerminator so that > this can be guaranteed. It's a little bit hairy because OnChildExit is called in > order for the two objects on the UI thread, and both post to the FILE thread, > and AwBrowserTerminator then posts back to the UI thread. But the net result is > that this will come after the minidump has been moved to the crash directory. Thanks, I added a comment about this :) (please let me know if the comment isn't verbose enough). https://codereview.chromium.org/2833893002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:415: AwBrowserProcess.setWebViewPackageName(webViewPackageName); On 2017/04/20 20:32:46, gsennton wrote: > AwBrowserProcess.setWebViewPackageName must be called before a renderer crash > triggers minidump uploading. Are we sure that this is the case? (I assume so > since we are calling from startChromiumLocked, but just want to double-check). > > I should probably add a comment about this ;) Done, the render process is set up in configureChildProcessLauncher(). https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2833893002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:168: handleMinidumpsAndSetMetricsConsent(sWebViewPackageName, false /* updateMetricsConsent */); On 2017/04/24 08:27:16, gsennton wrote: > On 2017/04/21 16:18:04, Tobias Sargeant wrote: > > I think now we could get into a situation where we have more than one > AsyncTask > > posted by handleMinidumps running concurrently. > > Right, that might be undesirable ;) > I think any alternative solution would make this code more complex (through > synchronisation), though I guess even now we would have to argue about what > might happen if two of these AsyncTasks interleave (which is a complex > argument). > I guess we could have a variable with three states on which to synchronize: > states: > > Nothing_going_on > Thread_copying_minidumps > Thread_should_copy_minidumps_again > > code: > > AwBrowserProcess.handleMinidumps(): > if (state == Nothing_going_on): > post new AsyncTask > set state = Thread_copying_minidumps > else if (state == Thread_copying_minidumps): > set state = Thread_should_copy_minidumps_again > > copyMinidumps() (in the AsyncTask): > copy minidumps... > before exiting: > if (state == Thread_should_copy_minidumps_again): > state = Thread_copying_minidumps > rerun minidump-copying > > > WDYT? > (this logic is becoming complex enough that I would like some tests for it > though... so would probably want to create some independent class for this logic > itself, and then test that separately). As Toby pointed out offline, if we just post these copy-tasks to the same thread we won't ever have any problem with synchronization (so I'm changing the handleMinidumps() call to post these tasks to AsyncTask.SERIAL_EXECUTOR).
https://codereview.chromium.org/2833893002/diff/20001/android_webview/native/... File android_webview/native/aw_browser_process_impl.cc (right): https://codereview.chromium.org/2833893002/diff/20001/android_webview/native/... android_webview/native/aw_browser_process_impl.cc:10: void TriggerMinidumpUploading() { Given that there's no abstract class and its implementation here, why is this split up into an _impl file?
https://codereview.chromium.org/2833893002/diff/20001/android_webview/native/... File android_webview/native/aw_browser_process_impl.cc (right): https://codereview.chromium.org/2833893002/diff/20001/android_webview/native/... android_webview/native/aw_browser_process_impl.cc:10: void TriggerMinidumpUploading() { On 2017/04/26 13:29:00, Tobias Sargeant wrote: > Given that there's no abstract class and its implementation here, why is this > split up into an _impl file? Good point ;), rename, and removed the empty browser/ implementation.
gsennton@chromium.org changed reviewers: + boliu@chromium.org
Hey Bo! Toby suggested we add you on this CL to clear up this layering question: We want to call a Jni method (TriggerMinidumpUploads) from aw_browser_terminator which lives in android_webview/browser/. android_webview/browser/ explicitly does not have access to JNI-methods to avoid spaghetti code/dependencies (android_webview/native/ has access to JNI-code, and is considered a higher layer than android_webview/browser/). The way we currently call into Java from android_webview/browser/ is through JniDependencyFactory which creates objects implemented in android_webview/native/, that do have access to Java. The current suggestions to solve the jni-dependency here are: 1. make triggerMinidumpUploads() a static method in JniDependencyFactory - this seems a bit ugly but is easier than the other suggestions. 2. create a new method in JniDependencyFactory that creates a temporary object which has a method calling the jni - triggerMinidumpUploads() method. 3. create a new object without the JniDependencyFactory, which we pass alongside JniDependencyFactory, and which has a method calling into the jni triggerMinidumpUploads methods (atm this doesn't seem to make sense, in my mind, compared to option 2 but I might have missed something just now ;)). WDYT?
On 2017/04/28 13:45:42, gsennton wrote: > Hey Bo! Toby suggested we add you on this CL to clear up this layering question: > > We want to call a Jni method (TriggerMinidumpUploads) from aw_browser_terminator > which lives in android_webview/browser/. > > android_webview/browser/ explicitly does not have access to JNI-methods to avoid > spaghetti code/dependencies (android_webview/native/ has access to JNI-code, and > is considered a higher layer than android_webview/browser/). > > The way we currently call into Java from android_webview/browser/ is through > JniDependencyFactory which creates objects implemented in > android_webview/native/, that do have access to Java. > The current suggestions to solve the jni-dependency here are: > 1. make triggerMinidumpUploads() a static method in JniDependencyFactory - this > seems a bit ugly but is easier than the other suggestions. > 2. create a new method in JniDependencyFactory that creates a temporary object > which has a method calling the jni - triggerMinidumpUploads() method. > 3. create a new object without the JniDependencyFactory, which we pass alongside > JniDependencyFactory, and which has a method calling into the jni > triggerMinidumpUploads methods (atm this doesn't seem to make sense, in my mind, > compared to option 2 but I might have missed something just now ;)). > > WDYT? I'm actually leaning towards removing that DEPS altogether, since I don't think splitting browser and native buys us anything. But let's see what others think. I'll start a thread on the public android-webview-dev
Okidoke, I've rebased this over Shimi's native/ -> browser/ CL now. I'm still letting the jni-calling implementation live in the "native" component for now.
On 2017/05/11 12:55:48, gsennton wrote: > Okidoke, I've rebased this over Shimi's native/ -> browser/ CL now. I'm still > letting the jni-calling implementation live in the "native" component for now. should probably merge the components in gn as well, although I didn't look at it that carefully
https://codereview.chromium.org/2833893002/diff/60001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2833893002/diff/60001/android_webview/BUILD.g... android_webview/BUILD.gn:553: "browser/aw_browser_process.h", Should this belong with the .cc file?
https://codereview.chromium.org/2833893002/diff/60001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2833893002/diff/60001/android_webview/BUILD.g... android_webview/BUILD.gn:553: "browser/aw_browser_process.h", On 2017/05/11 14:11:17, Tobias Sargeant wrote: > Should this belong with the .cc file? Yeo :P, done.
Rebased, ptal Toby :)
On 2017/05/22 19:56:38, gsennton wrote: > Rebased, ptal Toby :) LGTM
The CQ bit was checked by gsennton@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495538792264780,
"parent_rev": "f23b100a894de093b27aa2bef43165d5e8a68c3a", "commit_rev":
"9c0a1e4d561f51b8aa0861a952b0612934f9bbe6"}
Message was sent while issue was closed.
Description was changed from ========== [Android WebView] Trigger minidump uploading on renderer crash. In MultiProcess mode the WebView renderer process can crash without bringing down the browser process. Such a crash causes a minidump to be generated - with this CL that minidump is handled (copied, later to be uploaded) quickly after the renderer process crash instead of during the next WebView startup. BUG=674145 ========== to ========== [Android WebView] Trigger minidump uploading on renderer crash. In MultiProcess mode the WebView renderer process can crash without bringing down the browser process. Such a crash causes a minidump to be generated - with this CL that minidump is handled (copied, later to be uploaded) quickly after the renderer process crash instead of during the next WebView startup. BUG=674145 Review-Url: https://codereview.chromium.org/2833893002 Cr-Commit-Position: refs/heads/master@{#473871} Committed: https://chromium.googlesource.com/chromium/src/+/9c0a1e4d561f51b8aa0861a952b0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9c0a1e4d561f51b8aa0861a952b0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
