Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1445233002:
Schedule an early upload of non-JNI Java crashes
DescriptionSchedule an early upload of non-JNI Java crashes
BUG=554641
Patch Set 1 #
Total comments: 2
Patch Set 2 : addres comments #
Total comments: 12
Patch Set 3 : Tab.java change #
Total comments: 1
Patch Set 4 : sync #Patch Set 5 : sync / rebase #
Total comments: 2
Messages
Total messages: 21 (2 generated)
acleung@chromium.org changed reviewers: + mimee@chromium.org, yfriedman@chromium.org
Yaron: This seems to work for a few random places that I threw an uncaught exception. Is there any reason why we shouldn't make an early attempt?
https://codereview.chromium.org/1445233002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:294: public static void tryUploadLastCrashDump(Context context) { Can you add a test for this? We have sanity check for tryUploadAllCrash so we should at least make it symmetric. (Furthermore, it would be nice to see that an upload service actually starts after a non-JNI java crash.)
https://codereview.chromium.org/1445233002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:294: public static void tryUploadLastCrashDump(Context context) { On 2015/11/16 21:52:53, mimee wrote: > Can you add a test for this? We have sanity check for tryUploadAllCrash so we > should at least make it symmetric. > Done. > (Furthermore, it would be nice to see that an upload service actually starts > after a non-JNI java crash.) Not sure what you mean. How do you want to 'see' it?
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( Couple thoughts: 1) I get that you want to kick it off early but can we delay execution for a bit to avoid contention? 2) This doesn't apply strictly to "early" uploads. It also happens for late ones. Should we disable uploads after deferred start up? What do other ports do? Do they have a watchdog that uploads as soon as a crash happens or on next launch? 3) mimee commented on the bug that the preferences for uploading are only set after deferred startup. Why is this ok?
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( On 2015/11/17 01:53:09, Yaron wrote: > Couple thoughts: > 1) I get that you want to kick it off early but can we delay execution for a bit > to avoid contention? > 2) This doesn't apply strictly to "early" uploads. It also happens for late > ones. Should we disable uploads after deferred start up? What do other ports do? > Do they have a watchdog that uploads as soon as a crash happens or on next > launch? > 3) mimee commented on the bug that the preferences for uploading are only set > after deferred startup. Why is this ok? For 2), it seems to be relying on upload_last to upload just the last one and leave the rest for startup batch upload. There is currently no watchdog for browser crashes. For 3) It is at deferred startup.https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java&l=663 Expected behavior: w/o privacy pref set, it doesn't upload anyway; with pref set to no, it doesn't upload, etc. I am not familiar with this exception catching, but if user pref is never available when the exception is caught, then it is not the right place to call an upload anyway. We might have to consider embedding user pref in crash report filename, for instance, to get around looking for pref. https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:298: } Like an integration or instrumentation test (that passes)?
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( > 1) I get that you want to kick it off early but can we delay execution for a bit > to avoid contention? What sort of contention are you worried about? For a browser crash, isn't all that is left to do is a Pop-up asking if the user would like to upload to feedback? > 2) This doesn't apply strictly to "early" uploads. It also happens for late > ones. Should we disable uploads after deferred start up? What do other ports do? > Do they have a watchdog that uploads as soon as a crash happens or on next > launch? On linux, the set up is actually very similar. Breakpad spawns a shell to invoke "wget" and post the minidump. > 3) mimee commented on the bug that the preferences for uploading are only set > after deferred startup. Why is this ok? I don't really follow, mimee's comment. I thought those shared pref that are lazily initialized and probably be done by the time FRE or it's checks are complete. @mimee: Can you elaborate? https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( > I am not familiar with this exception catching, but if user pref is never > available when the exception is caught, then it is not the right place to call > an upload anyway. We might have to consider embedding user pref in crash report > filename, for instance, to get around looking for pref. See the above comment. Can you explain why this posts an issue?
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:298: } On 2015/11/17 18:22:59, mimee wrote: > Like an integration or instrumentation test (that passes)? Hm. I was under the impression that we had that until I realized it is busted. :( https://code.google.com/p/chromium/issues/detail?id=280886 Fixing that is outside the scope of this CL unless you feel really strongly about it.
mimee@chromium.org changed reviewers: + jchinlee@chromium.org
cc-ing jao-ke. https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( On 2015/11/17 22:29:04, acleung1 wrote: > > 1) I get that you want to kick it off early but can we delay execution for a > bit > > to avoid contention? > > What sort of contention are you worried about? For a browser crash, isn't all > that is left to do is a Pop-up asking if the user would like to upload to > feedback? > > > > 2) This doesn't apply strictly to "early" uploads. It also happens for late > > ones. Should we disable uploads after deferred start up? What do other ports > do? > > Do they have a watchdog that uploads as soon as a crash happens or on next > > launch? > > On linux, the set up is actually very similar. Breakpad spawns a shell to invoke > "wget" and post the minidump. > > > 3) mimee commented on the bug that the preferences for uploading are only set > > after deferred startup. Why is this ok? > > I don't really follow, mimee's comment. I thought those shared pref that are > lazily initialized and probably be done by the time FRE or it's checks are > complete. > > @mimee: Can you elaborate? Okay so I was just told that disableCrashUpload() in deferredStartupHandler is likely only used for clusterfuzz (cf wants clusterfuzz-induced crashes to not get uploaded). So you might be right, the pref might have been set already. This may only interfere with cf. CC-ing jao-ke on it.
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( On 2015/11/18 22:50:25, mimee wrote: > On 2015/11/17 22:29:04, acleung1 wrote: > > > 1) I get that you want to kick it off early but can we delay execution for a > > bit > > > to avoid contention? > > > > What sort of contention are you worried about? For a browser crash, isn't all > > that is left to do is a Pop-up asking if the user would like to upload to > > feedback? > > > > > > > 2) This doesn't apply strictly to "early" uploads. It also happens for late > > > ones. Should we disable uploads after deferred start up? What do other ports > > do? > > > Do they have a watchdog that uploads as soon as a crash happens or on next > > > launch? > > > > On linux, the set up is actually very similar. Breakpad spawns a shell to > invoke > > "wget" and post the minidump. > > > > > 3) mimee commented on the bug that the preferences for uploading are only > set > > > after deferred startup. Why is this ok? > > > > I don't really follow, mimee's comment. I thought those shared pref that are > > lazily initialized and probably be done by the time FRE or it's checks are > > complete. > > > > @mimee: Can you elaborate? > > Okay so I was just told that disableCrashUpload() in deferredStartupHandler is > likely > only used for clusterfuzz (cf wants clusterfuzz-induced crashes to not get > uploaded). > > So you might be right, the pref might have been set already. This may only > interfere > with cf. CC-ing jao-ke on it. Not sure if I have the full context for this. 1407413006 added a cl arg for disabling crash dump upload. It seems that this switch is /currently/ only used for CF, but it's meant for anyone who wants to disable minidump upload but retain the minidumps themselves in order to do further separate processing. This is a different population from typical users. Is this a question of cl args vs. intents? Also, would it be correct and feasible to embed the preference information into the filename when it is created, e.g. upload-yes now/yes sometime/no ever; delete-yes now/yes sometime/no ever? That is, if a typical user selects "yes-upload [and delete]", then switches to "no-don't upload [and delete]", all minidumps created during the "yes" period will be uploaded on the next upload-try cycle, whereas those created during the "no" period will be deleted [ASAP]. An experimenting user, on the other hand, has more control over which to delete, independently of which to upload.
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( On 2015/11/17 22:29:04, acleung1 wrote: > > 1) I get that you want to kick it off early but can we delay execution for a > bit > > to avoid contention? > > What sort of contention are you worried about? For a browser crash, isn't all > that is left to do is a Pop-up asking if the user would like to upload to > feedback? > Yes, I don't know what contention I was worried about. We're crashing anyway :) > > > 2) This doesn't apply strictly to "early" uploads. It also happens for late > > ones. Should we disable uploads after deferred start up? What do other ports > do? > > Do they have a watchdog that uploads as soon as a crash happens or on next > > launch? > > On linux, the set up is actually very similar. Breakpad spawns a shell to invoke > "wget" and post the minidump. > sgt > > 3) mimee commented on the bug that the preferences for uploading are only set > > after deferred startup. Why is this ok? > > I don't really follow, mimee's comment. I thought those shared pref that are > lazily initialized and probably be done by the time FRE or it's checks are > complete. > > @mimee: Can you elaborate? Ok, yes it's reading a java pref to see if uploading is possible. That should be kept in sync with native separately. Or actually - my last question is does this firing mean we're definitely crashing? If say I'm on the async task thread pool and I crash - does this fire? Does the exception also bubble up and ensure that chrome crashes (I think so?). And lastly - if this causes the intent service to start uploading, does the browser process going away mean the intent service also goes away possibly interrupting upload? https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:295: public static void tryUploadLastCrashDump(Context context) { Nit: update Tab.handleTabCrash to use this function
https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/JavaExceptionReporter.java:33: MinidumpUploadService.tryUploadLastCrashDump( > Ok, yes it's reading a java pref to see if uploading is possible. That should be > kept in sync with native separately. Ok I understand the concern. I'll talk to jchinlee about this. I think it is ok to do a best effort attempt at this stage. The key thing is to make sure the default behavor (when things are not fully initialized yet) we are not uploading. > Or actually - my last question is does this firing mean we're definitely > crashing? If say I'm on the async task thread pool and I crash - does this fire? > Does the exception also bubble up and ensure that chrome crashes (I think so?). > Yes. We install this in the UI thread, AsyncTask should handle the result by raising an runtime exception in the UI thread. > And lastly - if this causes the intent service to start uploading, does the > browser process going away mean the intent service also goes away possibly > interrupting upload? Not by the definition of a service: "Once started, a service can run in the background indefinitely, even if the component that started it is destroyed" Just like the above comment, it still a best effort attempt. If not, there is still browser restart. I really want to start using the GCM to upload crashes (crbug.com/524117) instead of doing it ourselves. https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:295: public static void tryUploadLastCrashDump(Context context) { On 2015/11/19 19:06:55, Yaron wrote: > Nit: update Tab.handleTabCrash to use this function Done.
yaron/jchinlee: Is there anything else you'd like to see done now that we have 1461883003 to make sure we are not relying on onDeferredStartup to get things set up correctly?
On 2015/11/30 22:36:28, acleung1 wrote: > yaron/jchinlee: Is there anything else you'd like to see done now that we have > 1461883003 to make sure we are not relying on onDeferredStartup to get things > set up correctly? Have we laid out what the right behavior should be? (Alan---we started discussing this before: right behavior given different upload conditions/permissions, timing, etc.) If that's not yet determined, we may want to lay that out first.
On 2015/12/01 18:14:20, jchinlee wrote: > On 2015/11/30 22:36:28, acleung1 wrote: > > yaron/jchinlee: Is there anything else you'd like to see done now that we have > > 1461883003 to make sure we are not relying on onDeferredStartup to get things > > set up correctly? > > Have we laid out what the right behavior should be? (Alan---we started > discussing this before: right behavior given different upload > conditions/permissions, timing, etc.) If that's not yet determined, we may want > to lay that out first. Yaron's respond was to not upload if deferredStartup isn't complete just to be on the safe side. Keep in mind that currently JavaExceptionReporter is installed *after* native is initialized anyways so my change isn't going to capture any new crash (yet). Yes, we should should definitely draw up a complete table describing when we should or shouldn't upload/dump/delete dumps. Otherwise we are just going to keep adding more "shouldUploadNowThatXandYConditionIsMet()" in the Privacy manager.
Sounds good! On Tue, Dec 1, 2015 at 1:45 PM, <acleung@chromium.org> wrote: > On 2015/12/01 18:14:20, jchinlee wrote: > >> On 2015/11/30 22:36:28, acleung1 wrote: >> > yaron/jchinlee: Is there anything else you'd like to see done now that >> we >> > have > >> > 1461883003 to make sure we are not relying on onDeferredStartup to get >> > things > >> > set up correctly? >> > > Have we laid out what the right behavior should be? (Alan---we started >> discussing this before: right behavior given different upload >> conditions/permissions, timing, etc.) If that's not yet determined, we may >> > want > >> to lay that out first. >> > > Yaron's respond was to not upload if deferredStartup isn't complete just > to be > on the safe side. > > Keep in mind that currently JavaExceptionReporter is installed *after* > native is > initialized anyways so my change isn't going to capture any new crash > (yet). > > Yes, we should should definitely draw up a complete table describing when > we > should or shouldn't upload/dump/delete dumps. Otherwise we are just going > to > keep adding more "shouldUploadNowThatXandYConditionIsMet()" in the Privacy > manager. > > https://codereview.chromium.org/1445233002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1445233002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:297: context.startService(intent); this should probably have the same try/catch I just added in https://codereview.chromium.org/1490483002/
rebased. PTAL.
https://codereview.chromium.org/1445233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/1445233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:274: protected boolean isMinidumpCleanNeeded() { Where is this used? https://codereview.chromium.org/1445233002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1445233002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2434: MinidumpUploadService.tryUploadLastCrashDump(context); uhm, this isn't what I meant (but it's technically ok. What I wanted was that for any context.startService related to crash, we should catch the SecurityException |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
