Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1447213002:
Add stats to distinguish android renderer crashes. (Closed)
|
Created:
5 years, 1 month ago by Peter Wen Modified:
5 years ago CC:
chromium-reviews, sadrul, vmpstr+watch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, kalyank, acleung1, mimee Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd stats to distinguish android renderer crashes.
This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.
The difference in scope is effectively
the number of renderers that crash as its corresponding
tabs are all backgrounded. This transition time is one second before the OOM protection is dropped. Most,
if not all, such renderer crashes are due to the Android OOM
killer. Overall difference in proposed stability stats will
not be much different, and will be easily seen with the new
histogram.
Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.
Committed: https://crrev.com/587e705898fd262f83acd78eef13f5858f73c4e2
Cr-Commit-Position: refs/heads/master@{#361110}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix for review. #
Total comments: 11
Patch Set 3 : Updated OOM protected logic to match android. #Patch Set 4 : Clean up includes. #Patch Set 5 : Punting exit status change to make merging easier. #
Total comments: 8
Patch Set 6 : More detailed stats. #Patch Set 7 : Fix termination status zero value. #
Total comments: 4
Patch Set 8 : Better app states. #
Total comments: 2
Patch Set 9 : Remove comment and rebase. #
Messages
Total messages: 35 (9 generated)
wnwen@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, Could you take a look at this CL? Anyone in particular on crash team that would be able to review the crash_dump_manager_android changes, maybe mmentovai@? Thanks. Peter
yfriedman@chromium.org changed reviewers: + sievers@chromium.org
I think it would be good for sievers to take a look https://codereview.chromium.org/1447213002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1447213002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:330: || state == ApplicationState.HAS_PAUSED_ACTIVITIES; Should we be using this instead? When is the strong binding dropped? in onPause or onStop (I think the latter) https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_manager_android.h (right): https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.h:53: enum ExitStatus { Please use IntDef instead: http://developer.android.com/reference/android/support/annotation/IntDef.html https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.h:77: void OnChildExit(int child_process_id, Typically don't overload functions in C++: https://google.github.io/styleguide/cppguide.html#Function_Overloading For that matter, just update callers - there aren't many
PTAL. https://codereview.chromium.org/1447213002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1447213002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:330: || state == ApplicationState.HAS_PAUSED_ACTIVITIES; On 2015/11/17 02:02:45, Yaron wrote: > Should we be using this instead? When is the strong binding dropped? in onPause > or onStop (I think the latter) Right, we only call onForegroundSessionEnd if the new state is HAS_STOPPED_ACTIVITIES in ChromeApplication. This may increase the disparity between this new stat and the old, but we'll only know by how much once it is live. It is possible that Chrome activities may not be paused that often without being stopped. https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_manager_android.h (right): https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.h:53: enum ExitStatus { On 2015/11/17 02:02:45, Yaron wrote: > Please use IntDef instead: > http://developer.android.com/reference/android/support/annotation/IntDef.html Switched to using IntDef in TabWebContentsObserver. Is there an equivalent in C++? https://codereview.chromium.org/1447213002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.h:77: void OnChildExit(int child_process_id, On 2015/11/17 02:02:46, Yaron wrote: > Typically don't overload functions in C++: > https://google.github.io/styleguide/cppguide.html#Function_Overloading > > For that matter, just update callers - there aren't many Done.
https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); There might be other reasons that our application would be marked as 'foreground', for example if you have a Service that calls startForeground() such as MediaNotificationManager.java does for when there is media playing. https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:113: if (processWasOomProtected) { nit: Ok I see what you are trying to fix (I think), which is that our 'processWasOomProtected' is not accurate here. What this says is whether there was a binding with BIND_IMPORTANT, but that only means 'foreground process level when the *client is*' (the browser). (http://developer.android.com/reference/android/content/Context.html#BIND_IMPO...) So the way you have it now makes it slightly more confusing, since the notion of 'OOM protected' is still wrong. And this is all just for dealing with the corner case of transitioning into the background? Why is this in the order of seconds? Can't we just make it so that we instantly remove the important bindings or so when the application goes into the background and avoid the corner case? https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:96: if (recordExitStatus) { nit: style here and elsewhere in C++ code - |record_exit_status| etc. https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:97: int exitStatus = ExitStatus::COUNT; nit: no need to init since it's if-else. https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:103: UMA_HISTOGRAM_ENUMERATION("Tab.RendererDetailedExitStatus", nit: since this is also called for other process types (utility, gpu), can you also pass the |ProcessType| (process_type.h) in here? And then maybe DCHECK_IMPLIES(recordExitStatus, process_type == PROCESS_TYPE_RENDERER) or something like that.
https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:113: if (processWasOomProtected) { On 2015/11/17 22:28:53, sievers wrote: > nit: Ok I see what you are trying to fix (I think), which is that our > 'processWasOomProtected' is not accurate here. What this says is whether there > was a binding with BIND_IMPORTANT, but that only means 'foreground process level > when the *client is*' (the browser). > (http://developer.android.com/reference/android/content/Context.html#BIND_IMPO...) > > So the way you have it now makes it slightly more confusing, since the notion of > 'OOM protected' is still wrong. > > And this is all just for dealing with the corner case of transitioning into the > background? Why is this in the order of seconds? > Can't we just make it so that we instantly remove the important bindings or so > when the application goes into the background and avoid the corner case? Hmm looks like the delay is 1-second and not 5 as I had thought (BindingManagerImpl.DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS). But basically, the idea is to collect number of OOMs in spite of strong binding where the child has strong binding and chrome is in fg
Description was changed from ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers that crashes while Chrome is in the foreground. The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is on the order of seconds before the OOM protection is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually results in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. BUG=510327 ========== to ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers, and redefining the OOM protected flag to match Android's definition, where it is only valid if the application itself is in the foreground: http://developer.android.com/reference/android/content/Context.html#BIND_IMPO... The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is 1 second before the strong binding is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually resulted in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. BUG=510327 ==========
Description was changed from ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers, and redefining the OOM protected flag to match Android's definition, where it is only valid if the application itself is in the foreground: http://developer.android.com/reference/android/content/Context.html#BIND_IMPO... The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is 1 second before the strong binding is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually resulted in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. BUG=510327 ========== to ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers, and redefining the OOM protected flag to match Android's definition, where it is only valid if the application itself is in the foreground: http://developer.android.com/reference/android/content/Context.html#BIND_IMPO... The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is 1 second before the strong binding is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually resulted in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. BUG=510327 ==========
PTAL. Updated description. https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); On 2015/11/17 22:28:53, sievers wrote: > There might be other reasons that our application would be marked as > 'foreground', for example if you have a Service that calls startForeground() > such as MediaNotificationManager.java does for when there is media playing. Not sure how to best handle this situation where a service is in foreground but rest of app is not. Would ApplicationStatus.determineApplicationState return ApplicationState.HAS_RUNNING_ACTIVITIES? Or perhaps a service does not have a corresponding ActivityInfo? https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:113: if (processWasOomProtected) { On 2015/11/18 15:25:17, Yaron wrote: > On 2015/11/17 22:28:53, sievers wrote: > > nit: Ok I see what you are trying to fix (I think), which is that our > > 'processWasOomProtected' is not accurate here. What this says is whether there > > was a binding with BIND_IMPORTANT, but that only means 'foreground process > level > > when the *client is*' (the browser). > > > (http://developer.android.com/reference/android/content/Context.html#BIND_IMPO...) > > > > So the way you have it now makes it slightly more confusing, since the notion > of > > 'OOM protected' is still wrong. > > > > And this is all just for dealing with the corner case of transitioning into > the > > background? Why is this in the order of seconds? > > Can't we just make it so that we instantly remove the important bindings or so > > when the application goes into the background and avoid the corner case? > > Hmm looks like the delay is 1-second and not 5 as I had thought > (BindingManagerImpl.DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS). > > But basically, the idea is to collect number of OOMs in spite of strong binding > where the child has strong binding and chrome is in fg Since BIND_IMPORTANT only works when the app is in the foreground, I've updated the OOM protected condition to reflect that. WDYT? https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:96: if (recordExitStatus) { On 2015/11/17 22:28:53, sievers wrote: > nit: style here and elsewhere in C++ code - |record_exit_status| etc. Done. https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:97: int exitStatus = ExitStatus::COUNT; On 2015/11/17 22:28:53, sievers wrote: > nit: no need to init since it's if-else. Done. https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:103: UMA_HISTOGRAM_ENUMERATION("Tab.RendererDetailedExitStatus", On 2015/11/17 22:28:53, sievers wrote: > nit: since this is also called for other process types (utility, gpu), can you > also pass the |ProcessType| (process_type.h) in here? And then maybe > DCHECK_IMPLIES(recordExitStatus, process_type == PROCESS_TYPE_RENDERER) or > something like that. Done.
+acleung,mimee as they're also working on this pipeline.
Description was changed from ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers, and redefining the OOM protected flag to match Android's definition, where it is only valid if the application itself is in the foreground: http://developer.android.com/reference/android/content/Context.html#BIND_IMPO... The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is 1 second before the strong binding is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually resulted in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. BUG=510327 ========== to ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers that crashes while Chrome is in the foreground. The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is one second before the OOM protection is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually results in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. ==========
On 2015/11/18 22:00:53, Peter Wen wrote: > PTAL. Updated description. > > https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java > (right): > > https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: > boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); > On 2015/11/17 22:28:53, sievers wrote: > > There might be other reasons that our application would be marked as > > 'foreground', for example if you have a Service that calls startForeground() > > such as MediaNotificationManager.java does for when there is media playing. > > Not sure how to best handle this situation where a service is in foreground but > rest of app is not. Would ApplicationStatus.determineApplicationState return > ApplicationState.HAS_RUNNING_ACTIVITIES? Or perhaps a service does not have a > corresponding ActivityInfo? > > https://codereview.chromium.org/1447213002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:113: > if (processWasOomProtected) { > On 2015/11/18 15:25:17, Yaron wrote: > > On 2015/11/17 22:28:53, sievers wrote: > > > nit: Ok I see what you are trying to fix (I think), which is that our > > > 'processWasOomProtected' is not accurate here. What this says is whether > there > > > was a binding with BIND_IMPORTANT, but that only means 'foreground process > > level > > > when the *client is*' (the browser). > > > > > > (http://developer.android.com/reference/android/content/Context.html#BIND_IMPO...) > > > > > > So the way you have it now makes it slightly more confusing, since the > notion > > of > > > 'OOM protected' is still wrong. > > > > > > And this is all just for dealing with the corner case of transitioning into > > the > > > background? Why is this in the order of seconds? > > > Can't we just make it so that we instantly remove the important bindings or > so > > > when the application goes into the background and avoid the corner case? > > > > Hmm looks like the delay is 1-second and not 5 as I had thought > > (BindingManagerImpl.DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS). > > > > But basically, the idea is to collect number of OOMs in spite of strong > binding > > where the child has strong binding and chrome is in fg > > Since BIND_IMPORTANT only works when the app is in the foreground, I've updated > the OOM protected condition to reflect that. WDYT? > > https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... > File components/crash/content/browser/crash_dump_manager_android.cc (right): > > https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... > components/crash/content/browser/crash_dump_manager_android.cc:96: if > (recordExitStatus) { > On 2015/11/17 22:28:53, sievers wrote: > > nit: style here and elsewhere in C++ code - |record_exit_status| etc. > > Done. > > https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... > components/crash/content/browser/crash_dump_manager_android.cc:97: int > exitStatus = ExitStatus::COUNT; > On 2015/11/17 22:28:53, sievers wrote: > > nit: no need to init since it's if-else. > > Done. > > https://codereview.chromium.org/1447213002/diff/20001/components/crash/conten... > components/crash/content/browser/crash_dump_manager_android.cc:103: > UMA_HISTOGRAM_ENUMERATION("Tab.RendererDetailedExitStatus", > On 2015/11/17 22:28:53, sievers wrote: > > nit: since this is also called for other process types (utility, gpu), can you > > also pass the |ProcessType| (process_type.h) in here? And then maybe > > DCHECK_IMPLIES(recordExitStatus, process_type == PROCESS_TYPE_RENDERER) or > > something like that. > > Done. Reverted exit status change. Will do a follow-up CL to streamline/simplify that, so this CL can be less risky when merging to M48. ATM this CL only adds more stat tracking and plumbs some values through to make that happen.
you'll also need a metrics owner for the actual histogram file changes https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); please move up to when activityState is captured https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:115: rendererExitStatus = TAB_RENDERER_EXIT_STATUS_OOM_PROTECTED_IN_FOREGROUND_APP; per discussion - can we split this out to visible vs paused https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:98: DCHECK(process_type == content::PROCESS_TYPE_RENDERER); DCHECK_EQ https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:99: int exitStatus; exit_status
Description was changed from ========== Add stats to distinguish renderer OOMs. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers that crashes while Chrome is in the foreground. The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is one second before the OOM protection is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually results in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. ========== to ========== Add stats to distinguish android renderer crashes. This CL is a precursor to expanding Clank stability scope to all OOM protected renderers that crash while Chrome is in the foreground. The difference in scope is effectively the number of renderers that crash as its corresponding tabs are all backgrounded. This transition time is one second before the OOM protection is dropped. Most, if not all, such renderer crashes are due to the Android OOM killer. Overall difference in proposed stability stats will not be much different, and will be easily seen with the new histogram. Expanding scope allows us to have native record which of the renderer crashes actually results in a minidump file being generated, thus demystifying whether the renderer was OOM killed or actually crashed. This native stat can be made more robust in the future, but this CL paves the foundation, assuming Tab.RendererExitStatus is comparable to Tab.RendererCrashStatus. ==========
Updated description to reflect stat changes. https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); On 2015/11/19 18:57:05, Yaron wrote: > please move up to when activityState is captured Done. https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:115: rendererExitStatus = TAB_RENDERER_EXIT_STATUS_OOM_PROTECTED_IN_FOREGROUND_APP; On 2015/11/19 18:57:05, Yaron wrote: > per discussion - can we split this out to visible vs paused Done. https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:98: DCHECK(process_type == content::PROCESS_TYPE_RENDERER); On 2015/11/19 18:57:05, Yaron wrote: > DCHECK_EQ Acknowledged. https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:99: int exitStatus; On 2015/11/19 18:57:05, Yaron wrote: > exit_status Done.
Updated description to reflect stat changes. https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:111: boolean applicationInForeground = ApplicationStatus.hasVisibleActivities(); On 2015/11/19 18:57:05, Yaron wrote: > please move up to when activityState is captured Done. https://codereview.chromium.org/1447213002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:115: rendererExitStatus = TAB_RENDERER_EXIT_STATUS_OOM_PROTECTED_IN_FOREGROUND_APP; On 2015/11/19 18:57:05, Yaron wrote: > per discussion - can we split this out to visible vs paused Done. https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:98: DCHECK(process_type == content::PROCESS_TYPE_RENDERER); On 2015/11/19 18:57:05, Yaron wrote: > DCHECK_EQ Acknowledged. https://codereview.chromium.org/1447213002/diff/80001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:99: int exitStatus; On 2015/11/19 18:57:05, Yaron wrote: > exit_status Done.
lgtm
wnwen@chromium.org changed reviewers: + cpu@chromium.org, isherman@chromium.org
+cpu@ for OWNERS components/crash/* +isherman@ for OWNERS tools/metrics/*
lgtm with nit https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:90: int app_state) { can you change int to |ApplicationState|? and then introduce _UNKNOWN and use it instead of 0. https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:195: base::android::ApplicationStatusListener::GetState()); why pass GetState() and not call it where you are using it above? that way there's not risk of using the wrong value.
https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:90: int app_state) { On 2015/11/19 20:43:27, sievers wrote: > can you change int to |ApplicationState|? and then introduce _UNKNOWN and use it > instead of 0. Done. https://codereview.chromium.org/1447213002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:195: base::android::ApplicationStatusListener::GetState()); On 2015/11/19 20:43:27, sievers wrote: > why pass GetState() and not call it where you are using it above? > that way there's not risk of using the wrong value. We want the application state as close to when the renderer died as possible, as the state is likely to change the longer we wait. Since ProcessMinidump is run on the FILE thread, it may be blocked for a while, making the stat less accurate. It is also only needed for the renderer, so I chose to call and pass it here instead of OnChildExit.
histograms lgtm
lgtm https://codereview.chromium.org/1447213002/diff/140001/components/crash/conte... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/140001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:163: /* exit_status */ base::TERMINATION_STATUS_MAX_ENUM, this inline comment style is not used before here or in general, consider removing it.
Thanks for the quick reviews! https://codereview.chromium.org/1447213002/diff/140001/components/crash/conte... File components/crash/content/browser/crash_dump_manager_android.cc (right): https://codereview.chromium.org/1447213002/diff/140001/components/crash/conte... components/crash/content/browser/crash_dump_manager_android.cc:163: /* exit_status */ base::TERMINATION_STATUS_MAX_ENUM, On 2015/11/20 22:31:38, cpu wrote: > this inline comment style is not used before here or in general, consider > removing it. Done.
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, sievers@chromium.org, cpu@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1447213002/#ps160001 (title: "Remove comment and rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1447213002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1447213002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/587e705898fd262f83acd78eef13f5858f73c4e2 Cr-Commit-Position: refs/heads/master@{#361110}
Message was sent while issue was closed.
For posterity, forgot bug link in CL. http://crbug.com/510327
Message was sent while issue was closed.
This seems to be breaking all perf tests, as soon as a renderer hits OOM https://code.google.com/p/chromium/issues/detail?id=560813#c3 As a side effect of this CL (see in crbug.com/560813 #3) the browser now suicides as soon as a renderer death is detected. I think the culprit here is rph==nullptr in CrashDumpManager::Observe
Message was sent while issue was closed.
Had an offline chat with yfriedman, reverting this in the meanwhile to get the waterfall green.
Message was sent while issue was closed.
On 2015/11/24 15:41:45, Primiano Tucci wrote: > Had an offline chat with yfriedman, reverting this in the meanwhile to get the > waterfall green. I don't think it's a null RPH. That was previously used on L145. From the bug it looks to be crashing on L194 - do you think that's accurate? I"m not sure how ApplicationStatusListener could be null. It seems possible that we don't have a RendererClosedDetails so it's crashing on the dereference on L193?
Message was sent while issue was closed.
Relanding here for posterity: http://crrev.com/1473753002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
