Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862353003:
[Crash Reporting] Fix a race between uploading and appending logcat data. (Closed)
|
Created:
3 years, 7 months ago by Ilya Sherman Modified:
3 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, agrieve+watch_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, kalyank, android-webview-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Crash Reporting] Fix a race between uploading and appending logcat data.
Prior to this CL, files ending with ".dmp" could potentially be simulatenously
scheduled for appending logcat data and scheduled for upload. This CL resolves
the race by requiring a ".try0" suffix to be present prior to attempting an
upload.
BUG=719129
R=gsennton@chromium.org
Review-Url: https://codereview.chromium.org/2862353003
Cr-Commit-Position: refs/heads/master@{#473319}
Committed: https://chromium.googlesource.com/chromium/src/+/d4b0825f3059a63da4210b9e8fa1d53271d60b0d
Patch Set 1 : #
Total comments: 31
Patch Set 2 : Rebase #Patch Set 3 : Migrate already copied Webview minidumps, loosen uploaded file regex, and add some tests #
Total comments: 17
Patch Set 4 : Drop the silly MIME stuff #Patch Set 5 : Fix some typos in comments #
Total comments: 18
Patch Set 6 : Simplify the WebView code a bit more, and rename MissingLogcat -> SansLogcat #Patch Set 7 : Rebase #Messages
Total messages: 48 (27 generated)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks for the CL Ilya! Please read my comments in CrashFileManager + MinidumpUploaderImpl before addressing any other the comments (I'm not sure about all the regex changes so to avoid you changing code several times it might be good to solidify those first :)). https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:169: // TODO(isherman): This is temporary migration logic, and can be removed in M61: I'm not sure we can remove this in 61 - what if someone takes an update directly from e.g. 59 to 61? (if the idea here is that in 60 all minidumps will be renamed, so that in 61 they will all be named with the new scheme). https://codereview.chromium.org/2862353003/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2862353003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:104: File minidumpToCopy = new File(getExistingCacheDir(), "toCopy.dmp.try0"); Maybe add a test ensuring that we can't upload files without the try0-suffix? https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:42: if (path != null && CrashFileManager.isMinidumpWithMimeTypeMissingLogcat(path)) { TODO I assume the file is renamed on failure as well? https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (left): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:511: // probably be removed around the M60 timeframe: http://crbug.com/699785 What happened to these histograms, shouldn't they be removed in a separate CL? https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:39: * 3 foo.try0 or foo.dmpNNNNN.try0 is a minidump file with recent logcat output attached to it; or You mean "foo.dmp" rather than "foo.try0" here, right? https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); Hmm, why are we changing whether to append a number to "dmp" here? I thought that signified the PIN of the crashing process rather than whether we had the MIME type? https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:84: Pattern.compile("\\.up([0-9]*)(\\.try([0-9]+))\\z"); I don't know whether we want to make this change here - there might still be uploaded files lying around from previous versions of Chrome/WebView, and with this change we won't pick those once up? For the READY_FOR_UPLOAD_PATTERN I think we did rename files with logcats appended to them by appending a "try0" to their name in 58 and 59 as well, so the reworking of the regexes above won't matter? (you might want to add a comment about this though). And I think we should go through all of the different regex-changes in this CL to ensure we aren't excluding any minidumps from previous (= 58/59) Chrome/WebView versions. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:99: File[] minidumps = fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED); Hmm, I'm not entirely sure about this - none of the minidumps that are stored in WebView's data directory across the WebView-update (the update including this CL) will be uploaded with this CL (I think), because they are missing the try0 suffix. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:152: fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED).length > 0; Same comment here as above.
Thanks for the review, Gustav! https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:169: // TODO(isherman): This is temporary migration logic, and can be removed in M61: On 2017/05/08 12:54:00, gsennton wrote: > I'm not sure we can remove this in 61 - what if someone takes an update directly > from e.g. 59 to 61? (if the idea here is that in 60 all minidumps will be > renamed, so that in 61 they will all be named with the new scheme). The idea is that by M61, any minidumps with the old naming convention will be so old that we do not care about them anymore. I guess that might not be entirely true, if we're interested in getting (recent) minidumps from users who skip over M60. I'm fine with bumping this several versions out, if you'd prefer. I do think that at some point, we just won't care about crash reports from M59- anymore =) https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:42: if (path != null && CrashFileManager.isMinidumpWithMimeTypeMissingLogcat(path)) { On 2017/05/08 12:54:00, gsennton wrote: > TODO I assume the file is renamed on failure as well? Right, the ProcessInitializationHandler should handle the failure case. https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (left): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:511: // probably be removed around the M60 timeframe: http://crbug.com/699785 On 2017/05/08 12:54:00, gsennton wrote: > What happened to these histograms, shouldn't they be removed in a separate CL? They're no longer as easily measurable, and I replaced them with documentation about the metrics (lines 535-550). But, yeah, I can remove them in a separate CL for extra emphasis. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:39: * 3 foo.try0 or foo.dmpNNNNN.try0 is a minidump file with recent logcat output attached to it; or On 2017/05/08 12:54:00, gsennton wrote: > You mean "foo.dmp" rather than "foo.try0" here, right? Ah, I mean foo.dmp.try0. Thanks for catching that! https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); On 2017/05/08 12:54:00, gsennton wrote: > Hmm, why are we changing whether to append a number to "dmp" here? I thought > that signified the PIN of the crashing process rather than whether we had the > MIME type? Honestly, I'm not 100% sure whether it's possible for the PID to be omitted, and what the relationship to the MIME type is. I've never quite figured out where the PID gets attached and whether it can actually fail sometimes, so I'm basing this code on my vague memory of comments that I've read in past CLs. The impression that I got from those was that MIME type info is somehow appended at the same time as the PID is. (Though, possibly just in a subset of circumstances? I'm not sure.) https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:84: Pattern.compile("\\.up([0-9]*)(\\.try([0-9]+))\\z"); On 2017/05/08 12:54:00, gsennton wrote: > I don't know whether we want to make this change here - there might still be > uploaded files lying around from previous versions of Chrome/WebView, and with > this change we won't pick those once up? Well, where does this regex actually get used? I think it's only for cleaning up old files... and we'll delete *all* old files past a certain age. So, I kinda figured it didn't matter if the crash dir had a few extra uploads sticking around for a month or so. WDYT? > For the READY_FOR_UPLOAD_PATTERN I think we did rename files with logcats > appended to them by appending a "try0" to their name in 58 and 59 as well, so > the reworking of the regexes above won't matter? (you might want to add a > comment about this though). Right, ".try0" has meant "has a logcat" since before I ever encountered the crash uploading code =) It's just that in 58 and 59, we would sometimes upload minidumps without that suffix, if logcat extraction failed. So files without the suffix were in an ambiguous state: sometimes they were intended for upload, whereas sometimes we still wanted to try to attach some logcat output to them first. What sort of a comment did you have in mind? I'm hesitant to add any comments like "The code used to do such and such prior to M60", as that sort of comment is not typically useful to readers of the code once the CL lands. But, maybe it would be useful in this case, or maybe you were thinking of a different sort of comment altogether? > And I think we should go through all of the different regex-changes in this CL > to ensure we aren't excluding any minidumps from previous (= 58/59) > Chrome/WebView versions. Agreed. I believe that ProcessInitializationHandler will rename all Chrome minidumps to have ".try0" appended if they never get logcat output added to them. I was hoping that the migration code in AwBrowserProcess would do the same for any pending WebView crashes. (Will address this in more detail in reply to your comment in MinidumpUploaderImpl.) https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:99: File[] minidumps = fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED); On 2017/05/08 12:54:00, gsennton wrote: > Hmm, I'm not entirely sure about this - none of the minidumps that are stored in > WebView's data directory across the WebView-update (the update including this > CL) will be uploaded with this CL (I think), because they are missing the try0 > suffix. Right, I meant to ask you about this, but then forgot. Thanks for bringing it up anyway! I was hoping that the AwBrowserProcess migration logic would rename all of the relevant files to include the ".try0" suffix. I guess I'm still not entirely sure about the lifecycle for WebView minidumps. I know they start in an app-specific directory, and then get copied over to the shared WebView directory. Is the AwBrowserProcess code responsible for the app dir or the shared dir? Is there an additional place where I could/should add some migration code, to cover all of the bases?
https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:169: // TODO(isherman): This is temporary migration logic, and can be removed in M61: On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > I'm not sure we can remove this in 61 - what if someone takes an update > directly > > from e.g. 59 to 61? (if the idea here is that in 60 all minidumps will be > > renamed, so that in 61 they will all be named with the new scheme). > > The idea is that by M61, any minidumps with the old naming convention will be so > old that we do not care about them anymore. I guess that might not be entirely > true, if we're interested in getting (recent) minidumps from users who skip over > M60. I'm fine with bumping this several versions out, if you'd prefer. I do > think that at some point, we just won't care about crash reports from M59- > anymore =) Yep, cool we don't need to decide right now. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > Hmm, why are we changing whether to append a number to "dmp" here? I thought > > that signified the PIN of the crashing process rather than whether we had the > > MIME type? > > Honestly, I'm not 100% sure whether it's possible for the PID to be omitted, and > what the relationship to the MIME type is. I've never quite figured out where > the PID gets attached and whether it can actually fail sometimes, so I'm basing > this code on my vague memory of comments that I've read in past CLs. The > impression that I got from those was that MIME type info is somehow appended at > the same time as the PID is. (Though, possibly just in a subset of > circumstances? I'm not sure.) Alright, I think we should try to figure out how this works :) I'll try to see if I can find it tomorrow. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:84: Pattern.compile("\\.up([0-9]*)(\\.try([0-9]+))\\z"); On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > I don't know whether we want to make this change here - there might still be > > uploaded files lying around from previous versions of Chrome/WebView, and with > > this change we won't pick those once up? > > Well, where does this regex actually get used? I think it's only for cleaning > up old files... and we'll delete *all* old files past a certain age. So, I > kinda figured it didn't matter if the crash dir had a few extra uploads sticking > around for a month or so. WDYT? > > > For the READY_FOR_UPLOAD_PATTERN I think we did rename files with logcats > > appended to them by appending a "try0" to their name in 58 and 59 as well, so > > the reworking of the regexes above won't matter? (you might want to add a > > comment about this though). > > Right, ".try0" has meant "has a logcat" since before I ever encountered the > crash uploading code =) It's just that in 58 and 59, we would sometimes upload > minidumps without that suffix, if logcat extraction failed. So files without > the suffix were in an ambiguous state: sometimes they were intended for upload, > whereas sometimes we still wanted to try to attach some logcat output to them > first. > > What sort of a comment did you have in mind? I'm hesitant to add any comments > like "The code used to do such and such prior to M60", as that sort of comment > is not typically useful to readers of the code once the CL lands. But, maybe it > would be useful in this case, or maybe you were thinking of a different sort of > comment altogether? > > > And I think we should go through all of the different regex-changes in this CL > > to ensure we aren't excluding any minidumps from previous (= 58/59) > > Chrome/WebView versions. > > Agreed. I believe that ProcessInitializationHandler will rename all Chrome > minidumps to have ".try0" appended if they never get logcat output added to > them. I was hoping that the migration code in AwBrowserProcess would do the > same for any pending WebView crashes. (Will address this in more detail in > reply to your comment in MinidumpUploaderImpl.) Do we actually need to change this pattern though, can't we just leave it as is? :) (even though it's only for minidump cleaning). (it would be nice to avoid having extra files around in the webview directory for months ;)). I can't remember what kind of comment I was thinking about atm, will come back to that tomorrow if it matters ;) https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:99: File[] minidumps = fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED); On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > Hmm, I'm not entirely sure about this - none of the minidumps that are stored > in > > WebView's data directory across the WebView-update (the update including this > > CL) will be uploaded with this CL (I think), because they are missing the try0 > > suffix. > > Right, I meant to ask you about this, but then forgot. Thanks for bringing it > up anyway! > > I was hoping that the AwBrowserProcess migration logic would rename all of the > relevant files to include the ".try0" suffix. I guess I'm still not entirely > sure about the lifecycle for WebView minidumps. I know they start in an > app-specific directory, and then get copied over to the shared WebView > directory. Is the AwBrowserProcess code responsible for the app dir or the > shared dir? Is there an additional place where I could/should add some > migration code, to cover all of the bases? Right, AwBrowserProcess is only responsible for (and only has access to) the app-specific directory. The shared directory can be touched by CrashReceiverService, AwMinidumpUploadJobService, or AwMinidumpUploaderDelegate (I think the latter two only operate on the UI thread so they shouldn't perform IO operations unless you kick off a separate thread to do that). Note that wherever you put this logic it might race with an already running uploading-job (so we just have to be careful to not crash if a file is renamed by another thread before we rename it). This shouldn't be run on the UI-thread (I think?), so you would either have to do it in one of the binder threads in CrashReceiverService, on the worker-thread in MinidumpUploaderImpl, or on a separate new thread. My preferred option would be the worker-thread in MinidumpUploaderImpl, but that code is shared with Chrome, so you would need some WebView-specific code to run >.<. You could add a temporary interface to MinidumpUploaderDelegate (and only implement it in AwMinidumpUploaderDelegate) :P
gsennton@chromium.org changed reviewers: + tobiasjs@chromium.org
Adding Tobias for visibility about MIME/crash related stuff. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); On 2017/05/09 19:23:52, gsennton wrote: > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > On 2017/05/08 12:54:00, gsennton wrote: > > > Hmm, why are we changing whether to append a number to "dmp" here? I thought > > > that signified the PIN of the crashing process rather than whether we had > the > > > MIME type? > > > > Honestly, I'm not 100% sure whether it's possible for the PID to be omitted, > and > > what the relationship to the MIME type is. I've never quite figured out where > > the PID gets attached and whether it can actually fail sometimes, so I'm > basing > > this code on my vague memory of comments that I've read in past CLs. The > > impression that I got from those was that MIME type info is somehow appended > at > > the same time as the PID is. (Though, possibly just in a subset of > > circumstances? I'm not sure.) > > Alright, I think we should try to figure out how this works :) > I'll try to see if I can find it tomorrow. This is where we append the PID: https://cs.chromium.org/chromium/src/components/crash/content/browser/crash_d... i.e. it happens when a renderer crashes and the browser moves the crash-file generated by the renderer, into our crash-directory. I talked to Toby about this - both Browser and Renderer crashes will include MIME-data, so whether or not we have a PID should be completely unrelated to whether the minidump filename contains a PID. The reason we use this pattern in MinidumpDirectoryObserver should be that MinidumpDirectoryObserver only really applies to renderer-crashes - since if the browser crashes the MinidumpDirectoryObserver will die as well (since it runs in the browser process). So AFAICT we shouldn't be using this pattern for anything else than trigger minidump-uploads from MinidumpDirectoryObserver. And using this pattern (through CrashFileManager.isMinidumpMIMEFirstTry()) in ProcessInitializationHandler.java to determine whether a logcat has been appended to a minidump is incorrect (AFAICT). The way I interpret the code right now we won't ever append logcats to browser minidumps (because doesCrashMinidumpNeedLogcat() will always return false for browser-crash-minidumps). Does this make sense?
https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:84: Pattern.compile("\\.up([0-9]*)(\\.try([0-9]+))\\z"); On 2017/05/09 19:23:52, gsennton wrote: > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > On 2017/05/08 12:54:00, gsennton wrote: > > > I don't know whether we want to make this change here - there might still be > > > uploaded files lying around from previous versions of Chrome/WebView, and > with > > > this change we won't pick those once up? > > > > Well, where does this regex actually get used? I think it's only for cleaning > > up old files... and we'll delete *all* old files past a certain age. So, I > > kinda figured it didn't matter if the crash dir had a few extra uploads > sticking > > around for a month or so. WDYT? > > > > > For the READY_FOR_UPLOAD_PATTERN I think we did rename files with logcats > > > appended to them by appending a "try0" to their name in 58 and 59 as well, > so > > > the reworking of the regexes above won't matter? (you might want to add a > > > comment about this though). > > > > Right, ".try0" has meant "has a logcat" since before I ever encountered the > > crash uploading code =) It's just that in 58 and 59, we would sometimes > upload > > minidumps without that suffix, if logcat extraction failed. So files without > > the suffix were in an ambiguous state: sometimes they were intended for > upload, > > whereas sometimes we still wanted to try to attach some logcat output to them > > first. > > > > What sort of a comment did you have in mind? I'm hesitant to add any comments > > like "The code used to do such and such prior to M60", as that sort of comment > > is not typically useful to readers of the code once the CL lands. But, maybe > it > > would be useful in this case, or maybe you were thinking of a different sort > of > > comment altogether? > > > > > And I think we should go through all of the different regex-changes in this > CL > > > to ensure we aren't excluding any minidumps from previous (= 58/59) > > > Chrome/WebView versions. > > > > Agreed. I believe that ProcessInitializationHandler will rename all Chrome > > minidumps to have ".try0" appended if they never get logcat output added to > > them. I was hoping that the migration code in AwBrowserProcess would do the > > same for any pending WebView crashes. (Will address this in more detail in > > reply to your comment in MinidumpUploaderImpl.) > > Do we actually need to change this pattern though, can't we just leave it as is? > :) (even though it's only for minidump cleaning). (it would be nice to avoid > having extra files around in the webview directory for months ;)). > > I can't remember what kind of comment I was thinking about atm, will come back > to that tomorrow if it matters ;) Regarding the comment - as you point out - it doesn't make sense to add a comment about the behaviour in 58/59 given that the behaviour back then won't affect the code after this CL (it will only affect whether we uploaded some minidumps with or without logcats at that time AFAICT) :).
Thanks for the thoughtful and thorough review, Gustav! Took me a while to gather my thoughts and address all of them, but here's a new revision up finally =) https://codereview.chromium.org/2862353003/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2862353003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:104: File minidumpToCopy = new File(getExistingCacheDir(), "toCopy.dmp.try0"); On 2017/05/08 12:54:00, gsennton wrote: > Maybe add a test ensuring that we can't upload files without the try0-suffix? Done. (In MinidumpUploaderImplTest and MinidumpUploadServiceTest -- the file name changes in this file are not actually required, but are rather more for consistency.) https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (left): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:511: // probably be removed around the M60 timeframe: http://crbug.com/699785 On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > What happened to these histograms, shouldn't they be removed in a separate CL? > > They're no longer as easily measurable, and I replaced them with documentation > about the metrics (lines 535-550). But, yeah, I can remove them in a separate > CL for extra emphasis. Removing them in https://codereview.chromium.org/2874383003/ https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:39: * 3 foo.try0 or foo.dmpNNNNN.try0 is a minidump file with recent logcat output attached to it; or On 2017/05/09 08:03:03, Ilya Sherman wrote: > On 2017/05/08 12:54:00, gsennton wrote: > > You mean "foo.dmp" rather than "foo.try0" here, right? > > Ah, I mean foo.dmp.try0. Thanks for catching that! Done. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); On 2017/05/10 11:23:29, gsennton wrote: > On 2017/05/09 19:23:52, gsennton wrote: > > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > > On 2017/05/08 12:54:00, gsennton wrote: > > > > Hmm, why are we changing whether to append a number to "dmp" here? I > thought > > > > that signified the PIN of the crashing process rather than whether we had > > the > > > > MIME type? > > > > > > Honestly, I'm not 100% sure whether it's possible for the PID to be omitted, > > and > > > what the relationship to the MIME type is. I've never quite figured out > where > > > the PID gets attached and whether it can actually fail sometimes, so I'm > > basing > > > this code on my vague memory of comments that I've read in past CLs. The > > > impression that I got from those was that MIME type info is somehow appended > > at > > > the same time as the PID is. (Though, possibly just in a subset of > > > circumstances? I'm not sure.) > > > > Alright, I think we should try to figure out how this works :) > > I'll try to see if I can find it tomorrow. > > This is where we append the PID: > https://cs.chromium.org/chromium/src/components/crash/content/browser/crash_d... > > i.e. it happens when a renderer crashes and the browser moves the crash-file > generated by the renderer, into our crash-directory. I talked to Toby about this > - both Browser and Renderer crashes will include MIME-data, so whether or not we > have a PID should be completely unrelated to whether the minidump filename > contains a PID. > > The reason we use this pattern in MinidumpDirectoryObserver should be that > MinidumpDirectoryObserver only really applies to renderer-crashes - since if the > browser crashes the MinidumpDirectoryObserver will die as well (since it runs in > the browser process). > > So AFAICT we shouldn't be using this pattern for anything else than trigger > minidump-uploads from MinidumpDirectoryObserver. And using this pattern (through > CrashFileManager.isMinidumpMIMEFirstTry()) in ProcessInitializationHandler.java > to determine whether a logcat has been appended to a minidump is incorrect > (AFAICT). The way I interpret the code right now we won't ever append logcats to > browser minidumps (because doesCrashMinidumpNeedLogcat() will always return > false for browser-crash-minidumps). Does this make sense? So, the file you linked to is where child crashes get a PID attached. I just tested manually, and the crashes from the browser process also have a PID attached. I tried finding where that happens, but my search-foo was not strong enough. FWIW, you can scan through crash reports uploaded for the browser process. They generally do have logcat data attached currently. (Also, it seems we name the crash file "chromium-renderer-minidump-foo..." regardless of the process type of the child process. Sigh.) I'm not sure at this point whether the term "MIME" is correct here, or whether it was always supposed to be "PID". Since the term was present before I started working on the code, I'm still quite confused by it. I agree with you that it's not clear why there's a relationship between MIME type info and PID info. I sent you a CL to just get rid of the MinidumpDirectoryObserver's spooky action at a distance: [ https://codereview.chromium.org/2878193002 ]. It's a bit of extra code to jump through JNI hoops and add a callback from the components layer. LMK whether you prefer this approach or keeping the MinidumpDirectoryObserver, and tweaking the regexes in some fashion. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:84: Pattern.compile("\\.up([0-9]*)(\\.try([0-9]+))\\z"); On 2017/05/09 19:23:52, gsennton wrote: > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > On 2017/05/08 12:54:00, gsennton wrote: > > > I don't know whether we want to make this change here - there might still be > > > uploaded files lying around from previous versions of Chrome/WebView, and > with > > > this change we won't pick those once up? > > > > Well, where does this regex actually get used? I think it's only for cleaning > > up old files... and we'll delete *all* old files past a certain age. So, I > > kinda figured it didn't matter if the crash dir had a few extra uploads > sticking > > around for a month or so. WDYT? > > > > > For the READY_FOR_UPLOAD_PATTERN I think we did rename files with logcats > > > appended to them by appending a "try0" to their name in 58 and 59 as well, > so > > > the reworking of the regexes above won't matter? (you might want to add a > > > comment about this though). > > > > Right, ".try0" has meant "has a logcat" since before I ever encountered the > > crash uploading code =) It's just that in 58 and 59, we would sometimes > upload > > minidumps without that suffix, if logcat extraction failed. So files without > > the suffix were in an ambiguous state: sometimes they were intended for > upload, > > whereas sometimes we still wanted to try to attach some logcat output to them > > first. > > > > What sort of a comment did you have in mind? I'm hesitant to add any comments > > like "The code used to do such and such prior to M60", as that sort of comment > > is not typically useful to readers of the code once the CL lands. But, maybe > it > > would be useful in this case, or maybe you were thinking of a different sort > of > > comment altogether? > > > > > And I think we should go through all of the different regex-changes in this > CL > > > to ensure we aren't excluding any minidumps from previous (= 58/59) > > > Chrome/WebView versions. > > > > Agreed. I believe that ProcessInitializationHandler will rename all Chrome > > minidumps to have ".try0" appended if they never get logcat output added to > > them. I was hoping that the migration code in AwBrowserProcess would do the > > same for any pending WebView crashes. (Will address this in more detail in > > reply to your comment in MinidumpUploaderImpl.) > > Do we actually need to change this pattern though, can't we just leave it as is? > :) (even though it's only for minidump cleaning). (it would be nice to avoid > having extra files around in the webview directory for months ;)). > > I can't remember what kind of comment I was thinking about atm, will come back > to that tomorrow if it matters ;) Okay, I reverted the change for now, and added a TODO to tighten the regex later. I do think that it's worth tightening eventually, as it makes the code more self-documenting. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:99: File[] minidumps = fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED); On 2017/05/09 19:23:52, gsennton wrote: > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > On 2017/05/08 12:54:00, gsennton wrote: > > > Hmm, I'm not entirely sure about this - none of the minidumps that are > stored > > in > > > WebView's data directory across the WebView-update (the update including > this > > > CL) will be uploaded with this CL (I think), because they are missing the > try0 > > > suffix. > > > > Right, I meant to ask you about this, but then forgot. Thanks for bringing it > > up anyway! > > > > I was hoping that the AwBrowserProcess migration logic would rename all of the > > relevant files to include the ".try0" suffix. I guess I'm still not entirely > > sure about the lifecycle for WebView minidumps. I know they start in an > > app-specific directory, and then get copied over to the shared WebView > > directory. Is the AwBrowserProcess code responsible for the app dir or the > > shared dir? Is there an additional place where I could/should add some > > migration code, to cover all of the bases? > > Right, AwBrowserProcess is only responsible for (and only has access to) the > app-specific directory. > > The shared directory can be touched by CrashReceiverService, > AwMinidumpUploadJobService, or AwMinidumpUploaderDelegate (I think the latter > two only operate on the UI thread so they shouldn't perform IO operations unless > you kick off a separate thread to do that). Note that wherever you put this > logic it might race with an already running uploading-job (so we just have to be > careful to not crash if a file is renamed by another thread before we rename > it). > > This shouldn't be run on the UI-thread (I think?), so you would either have to > do it in one of the binder threads in CrashReceiverService, on the worker-thread > in MinidumpUploaderImpl, or on a separate new thread. My preferred option would > be the worker-thread in MinidumpUploaderImpl, but that code is shared with > Chrome, so you would need some WebView-specific code to run >.<. You could add a > temporary interface to MinidumpUploaderDelegate (and only implement it in > AwMinidumpUploaderDelegate) :P Done. https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:152: fileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_TRIES_ALLOWED).length > 0; On 2017/05/08 12:54:00, gsennton wrote: > Same comment here as above. Acknowledged.
Thanks Ilya! A couple more comments: https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (left): https://codereview.chromium.org/2862353003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:511: // probably be removed around the M60 timeframe: http://crbug.com/699785 On 2017/05/13 06:35:04, Ilya Sherman wrote: > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > On 2017/05/08 12:54:00, gsennton wrote: > > > What happened to these histograms, shouldn't they be removed in a separate > CL? > > > > They're no longer as easily measurable, and I replaced them with documentation > > about the metrics (lines 535-550). But, yeah, I can remove them in a separate > > CL for extra emphasis. > > Removing them in https://codereview.chromium.org/2874383003/ Thanks! https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/60001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:68: Pattern.compile("\\.dmp([0-9]*)\\z"); On 2017/05/13 06:35:05, Ilya Sherman wrote: > On 2017/05/10 11:23:29, gsennton wrote: > > On 2017/05/09 19:23:52, gsennton wrote: > > > On 2017/05/09 08:03:03, Ilya Sherman wrote: > > > > On 2017/05/08 12:54:00, gsennton wrote: > > > > > Hmm, why are we changing whether to append a number to "dmp" here? I > > thought > > > > > that signified the PIN of the crashing process rather than whether we > had > > > the > > > > > MIME type? > > > > > > > > Honestly, I'm not 100% sure whether it's possible for the PID to be > omitted, > > > and > > > > what the relationship to the MIME type is. I've never quite figured out > > where > > > > the PID gets attached and whether it can actually fail sometimes, so I'm > > > basing > > > > this code on my vague memory of comments that I've read in past CLs. The > > > > impression that I got from those was that MIME type info is somehow > appended > > > at > > > > the same time as the PID is. (Though, possibly just in a subset of > > > > circumstances? I'm not sure.) > > > > > > Alright, I think we should try to figure out how this works :) > > > I'll try to see if I can find it tomorrow. > > > > This is where we append the PID: > > > https://cs.chromium.org/chromium/src/components/crash/content/browser/crash_d... > > > > i.e. it happens when a renderer crashes and the browser moves the crash-file > > generated by the renderer, into our crash-directory. I talked to Toby about > this > > - both Browser and Renderer crashes will include MIME-data, so whether or not > we > > have a PID should be completely unrelated to whether the minidump filename > > contains a PID. > > > > The reason we use this pattern in MinidumpDirectoryObserver should be that > > MinidumpDirectoryObserver only really applies to renderer-crashes - since if > the > > browser crashes the MinidumpDirectoryObserver will die as well (since it runs > in > > the browser process). > > > > So AFAICT we shouldn't be using this pattern for anything else than trigger > > minidump-uploads from MinidumpDirectoryObserver. And using this pattern > (through > > CrashFileManager.isMinidumpMIMEFirstTry()) in > ProcessInitializationHandler.java > > to determine whether a logcat has been appended to a minidump is incorrect > > (AFAICT). The way I interpret the code right now we won't ever append logcats > to > > browser minidumps (because doesCrashMinidumpNeedLogcat() will always return > > false for browser-crash-minidumps). Does this make sense? > > So, the file you linked to is where child crashes get a PID attached. I just > tested manually, and the crashes from the browser process also have a PID > attached. I tried finding where that happens, but my search-foo was not strong > enough. FWIW, you can scan through crash reports uploaded for the browser > process. They generally do have logcat data attached currently. (Also, it > seems we name the crash file "chromium-renderer-minidump-foo..." regardless of > the process type of the child process. Sigh.) > > I'm not sure at this point whether the term "MIME" is correct here, or whether > it was always supposed to be "PID". Since the term was present before I started > working on the code, I'm still quite confused by it. I agree with you that it's > not clear why there's a relationship between MIME type info and PID info. > > I sent you a CL to just get rid of the MinidumpDirectoryObserver's spooky action > at a distance: [ https://codereview.chromium.org/2878193002 ]. It's a bit of > extra code to jump through JNI hoops and add a callback from the components > layer. LMK whether you prefer this approach or keeping the > MinidumpDirectoryObserver, and tweaking the regexes in some fashion. I much prefer the new approach over the directory observer, thanks! https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:179: crashFileManager.getMinidumpsReadyForUpload(MAX_MINIDUMP_UPLOAD_TRIES); Potentially (as a solution to WebView-minidumps not being named with "try0", we could upload both the minidumps from getMinidumpsReadyForUpload() and the ones without logcats here (since all of our minidumps will be missing logcats). Would that make sense, or would it be too hacky? https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:77: private static final Pattern MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN = I still don't see the connection between the PID and the MIME type - so I'm still not sure about this pattern :/. Another naming question: I read MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN as "Minidump with mime type missing, logcat pattern" (I know that doesn't make sense but that's how it read out in my mind), if we stick with this pattern, could you maybe call it MINIDUMP_WITH_MIME_TYPE_AND_MISSING_LOGCAT_PATTERN ? :) https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:640: + READY_FOR_UPLOAD_SUFFIX; Oooh, right, so the change for this method ensures that minidumps copied to the separate WebView process are renamed correctly (ending with "try0"), but where does this happen in the app process? AwBrowserProcess migrates the old minidumps, but the new ones should be created with the "try0" suffix as well, right? Otherwise we'll have to keep the AwBrowserProcess migration logic forever :/ (sorry for not catching this earlier, I didn't think about the minidumps generated after this CL has landed). https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:151: mDelegate.migrateMinidumpFilenamesIfNeeded(fileManager); Hmm, so the first time we run this Job on a WebView version containing this CL we will call getMinidumpsReadyForUpload() above - and not retrieve any of the Minidumps from the older WebView versions since those don't include "tryN" (unless we've tried to upload them already). I guess this is OK as long as we migrate the files before we call the getMinidumpsReadyForUpload() again below to determine whether to reschedule (though we might 'waste' one job on just migrating old minidumps). This sounds fine given that our priority should be uploading minidumps from the new version anyway ;)
Thanks, Gustav. Just replying to comments for now. Will update the code once the two CLs this depends on (removing histograms and removing the minidump directory observer) land. https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:179: crashFileManager.getMinidumpsReadyForUpload(MAX_MINIDUMP_UPLOAD_TRIES); On 2017/05/15 13:47:59, gsennton wrote: > Potentially (as a solution to WebView-minidumps not being named with "try0", we > could upload both the minidumps from getMinidumpsReadyForUpload() and the ones > without logcats here (since all of our minidumps will be missing logcats). Would > that make sense, or would it be too hacky? Hmm, does the migration code immediately above this not accomplish pretty much that? What am I missing? =) https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:77: private static final Pattern MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN = On 2017/05/15 13:47:59, gsennton wrote: > I still don't see the connection between the PID and the MIME type - so I'm > still not sure about this pattern :/. > > Another naming question: I read MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN > as > "Minidump with mime type missing, logcat pattern" (I know that doesn't make > sense but that's how it read out in my mind), if we stick with this pattern, > could you maybe call it MINIDUMP_WITH_MIME_TYPE_AND_MISSING_LOGCAT_PATTERN ? :) I think I should be able to remove this pattern entirely in favor of the approach in https://codereview.chromium.org/2878193002/. Hooray! https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:640: + READY_FOR_UPLOAD_SUFFIX; On 2017/05/15 13:47:59, gsennton wrote: > Oooh, right, so the change for this method ensures that minidumps copied to the > separate WebView process are renamed correctly (ending with "try0"), but where > does this happen in the app process? AwBrowserProcess migrates the old > minidumps, but the new ones should be created with the "try0" suffix as well, > right? Otherwise we'll have to keep the AwBrowserProcess migration logic forever > :/ > (sorry for not catching this earlier, I didn't think about the minidumps > generated after this CL has landed). Hmm, where are those names generated? We should indeed update them too. Is that basically just the crash_dump_handler_android code path? If so, then yeah, the "migration" code in AwBrowserProcess shouldn't be temporary, same as for Chrome (but not because of logcats, as it is for Chrome). https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:151: mDelegate.migrateMinidumpFilenamesIfNeeded(fileManager); On 2017/05/15 13:47:59, gsennton wrote: > Hmm, so the first time we run this Job on a WebView version containing this CL > we will call getMinidumpsReadyForUpload() above - and not retrieve any of the > Minidumps from the older WebView versions since those don't include "tryN" > (unless we've tried to upload them already). I guess this is OK as long as we > migrate the files before we call the getMinidumpsReadyForUpload() again below to > determine whether to reschedule (though we might 'waste' one job on just > migrating old minidumps). This sounds fine given that our priority should be > uploading minidumps from the new version anyway ;) Yeah, that seems intuitively ok to me, since we do indeed care the most about new crashes. I suppose there is some concern that we wouldn't run this code *at all* if we don't see any crashes from the new version, so the upload of the old crashes might be really delayed. Is that the case? Sorry, I definitely wrote this CL primarily with Chrome in mind, and am doing a poor job of properly understanding and filling in the gaps for WebView =/
https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:179: crashFileManager.getMinidumpsReadyForUpload(MAX_MINIDUMP_UPLOAD_TRIES); On 2017/05/16 07:15:39, Ilya Sherman wrote: > On 2017/05/15 13:47:59, gsennton wrote: > > Potentially (as a solution to WebView-minidumps not being named with "try0", > we > > could upload both the minidumps from getMinidumpsReadyForUpload() and the ones > > without logcats here (since all of our minidumps will be missing logcats). > Would > > that make sense, or would it be too hacky? > > Hmm, does the migration code immediately above this not accomplish pretty much > that? What am I missing? =) It does, but the migration logic would have to stay here forever (you can't remove it M61) since we don't have the MinidumpPreparer (or w/e it's called) in WebView which renames minidumps after trying to append append the logcat to them. https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:77: private static final Pattern MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN = On 2017/05/16 07:15:39, Ilya Sherman wrote: > On 2017/05/15 13:47:59, gsennton wrote: > > I still don't see the connection between the PID and the MIME type - so I'm > > still not sure about this pattern :/. > > > > Another naming question: I read MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN > > as > > "Minidump with mime type missing, logcat pattern" (I know that doesn't make > > sense but that's how it read out in my mind), if we stick with this pattern, > > could you maybe call it MINIDUMP_WITH_MIME_TYPE_AND_MISSING_LOGCAT_PATTERN ? > :) > > I think I should be able to remove this pattern entirely in favor of the > approach in https://codereview.chromium.org/2878193002/. Hooray! And you don't need it to implement doesCrashMinidumpNeedLogcat()? (in your latest PS it's used there) https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:640: + READY_FOR_UPLOAD_SUFFIX; On 2017/05/16 07:15:39, Ilya Sherman wrote: > On 2017/05/15 13:47:59, gsennton wrote: > > Oooh, right, so the change for this method ensures that minidumps copied to > the > > separate WebView process are renamed correctly (ending with "try0"), but where > > does this happen in the app process? AwBrowserProcess migrates the old > > minidumps, but the new ones should be created with the "try0" suffix as well, > > right? Otherwise we'll have to keep the AwBrowserProcess migration logic > forever > > :/ > > (sorry for not catching this earlier, I didn't think about the minidumps > > generated after this CL has landed). > > Hmm, where are those names generated? We should indeed update them too. Is > that basically just the crash_dump_handler_android code path? If so, then yeah, > the "migration" code in AwBrowserProcess shouldn't be temporary, same as for > Chrome (but not because of logcats, as it is for Chrome). Yeah, those names should be generated in the same way as on Chrome (i.e. when the minidump is renamed just after being created). https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:151: mDelegate.migrateMinidumpFilenamesIfNeeded(fileManager); On 2017/05/16 07:15:39, Ilya Sherman wrote: > On 2017/05/15 13:47:59, gsennton wrote: > > Hmm, so the first time we run this Job on a WebView version containing this CL > > we will call getMinidumpsReadyForUpload() above - and not retrieve any of the > > Minidumps from the older WebView versions since those don't include "tryN" > > (unless we've tried to upload them already). I guess this is OK as long as we > > migrate the files before we call the getMinidumpsReadyForUpload() again below > to > > determine whether to reschedule (though we might 'waste' one job on just > > migrating old minidumps). This sounds fine given that our priority should be > > uploading minidumps from the new version anyway ;) > > Yeah, that seems intuitively ok to me, since we do indeed care the most about > new crashes. I suppose there is some concern that we wouldn't run this code *at > all* if we don't see any crashes from the new version, so the upload of the old > crashes might be really delayed. Is that the case? Sorry, I definitely wrote It depends on what you mean by really delayed - the worst thing that can happen is that we don't see any new crashes. In that case we will still have a job running (because of the old crashes), and that job will see that there are no new crashes (i.e. getMinidumpsReadyForUpload() returns an empty array), and thus skip through the for-loop above, and then call migrateMinidumpFilenamesIfNeeded(). The job will then be rescheduled because after the old minidumps are migrated the getMinidumpsReadyForUpload() call below will return a non-empty array. The next time the job runs it will upload the migrated files :) So to sum up: the first time we want to upload minidumps from the old WebView version we will need two jobs (the current one, and that one being rescheduled) instead of one job to upload those minidumps. > this CL primarily with Chrome in mind, and am doing a poor job of properly > understanding and filling in the gaps for WebView =/ No worries ;), WebView is a bit tricky here :P, I should make some slideshow or something showing this in a much more straight-forward way than the design doc - I'll try to think of a good way to present this (without the information being obsolete as soon as we change parts of the).
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Hokay, updated now that the depended-on CLs have landed. PTAL! https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:179: crashFileManager.getMinidumpsReadyForUpload(MAX_MINIDUMP_UPLOAD_TRIES); On 2017/05/16 10:25:49, gsennton wrote: > On 2017/05/16 07:15:39, Ilya Sherman wrote: > > On 2017/05/15 13:47:59, gsennton wrote: > > > Potentially (as a solution to WebView-minidumps not being named with "try0", > > we > > > could upload both the minidumps from getMinidumpsReadyForUpload() and the > ones > > > without logcats here (since all of our minidumps will be missing logcats). > > Would > > > that make sense, or would it be too hacky? > > > > Hmm, does the migration code immediately above this not accomplish pretty much > > that? What am I missing? =) > > It does, but the migration logic would have to stay here forever (you can't > remove it M61) since we don't have the MinidumpPreparer (or w/e it's called) in > WebView which renames minidumps after trying to append append the logcat to > them. Got it, that makes sense. Done. https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:77: private static final Pattern MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN = On 2017/05/16 10:25:49, gsennton wrote: > On 2017/05/16 07:15:39, Ilya Sherman wrote: > > On 2017/05/15 13:47:59, gsennton wrote: > > > I still don't see the connection between the PID and the MIME type - so I'm > > > still not sure about this pattern :/. > > > > > > Another naming question: I read > MINIDUMP_WITH_MIME_TYPE_MISSING_LOGCAT_PATTERN > > > as > > > "Minidump with mime type missing, logcat pattern" (I know that doesn't make > > > sense but that's how it read out in my mind), if we stick with this pattern, > > > could you maybe call it MINIDUMP_WITH_MIME_TYPE_AND_MISSING_LOGCAT_PATTERN ? > > :) > > > > I think I should be able to remove this pattern entirely in favor of the > > approach in https://codereview.chromium.org/2878193002/. Hooray! > > And you don't need it to implement doesCrashMinidumpNeedLogcat()? > (in your latest PS it's used there) Wow, that was a bug... I meant to use isMinidumpMissingLogcat() there. Good thing there's only one option now! (In practice, I'm not sure whether it's actually possible for the PID to be missing for *any* crashes. But since I'm not 100% sure, I'll go with the more forgiving implementation.) https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:640: + READY_FOR_UPLOAD_SUFFIX; On 2017/05/16 10:25:49, gsennton wrote: > On 2017/05/16 07:15:39, Ilya Sherman wrote: > > On 2017/05/15 13:47:59, gsennton wrote: > > > Oooh, right, so the change for this method ensures that minidumps copied to > > the > > > separate WebView process are renamed correctly (ending with "try0"), but > where > > > does this happen in the app process? AwBrowserProcess migrates the old > > > minidumps, but the new ones should be created with the "try0" suffix as > well, > > > right? Otherwise we'll have to keep the AwBrowserProcess migration logic > > forever > > > :/ > > > (sorry for not catching this earlier, I didn't think about the minidumps > > > generated after this CL has landed). > > > > Hmm, where are those names generated? We should indeed update them too. Is > > that basically just the crash_dump_handler_android code path? If so, then > yeah, > > the "migration" code in AwBrowserProcess shouldn't be temporary, same as for > > Chrome (but not because of logcats, as it is for Chrome). > > Yeah, those names should be generated in the same way as on Chrome (i.e. when > the minidump is renamed just after being created). Acknowledged. https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:151: mDelegate.migrateMinidumpFilenamesIfNeeded(fileManager); On 2017/05/16 10:25:49, gsennton wrote: > On 2017/05/16 07:15:39, Ilya Sherman wrote: > > On 2017/05/15 13:47:59, gsennton wrote: > > > Hmm, so the first time we run this Job on a WebView version containing this > CL > > > we will call getMinidumpsReadyForUpload() above - and not retrieve any of > the > > > Minidumps from the older WebView versions since those don't include "tryN" > > > (unless we've tried to upload them already). I guess this is OK as long as > we > > > migrate the files before we call the getMinidumpsReadyForUpload() again > below > > to > > > determine whether to reschedule (though we might 'waste' one job on just > > > migrating old minidumps). This sounds fine given that our priority should be > > > uploading minidumps from the new version anyway ;) > > > > Yeah, that seems intuitively ok to me, since we do indeed care the most about > > new crashes. I suppose there is some concern that we wouldn't run this code > *at > > all* if we don't see any crashes from the new version, so the upload of the > old > > crashes might be really delayed. Is that the case? Sorry, I definitely wrote > > It depends on what you mean by really delayed - the worst thing that can happen > is that we don't see any new crashes. In that case we will still have a job > running (because of the old crashes), and that job will see that there are no > new crashes (i.e. getMinidumpsReadyForUpload() returns an empty array), and thus > skip through the for-loop above, and then call > migrateMinidumpFilenamesIfNeeded(). The job will then be rescheduled because > after the old minidumps are migrated the getMinidumpsReadyForUpload() call below > will return a non-empty array. > The next time the job runs it will upload the migrated files :) > > So to sum up: the first time we want to upload minidumps from the old WebView > version we will need two jobs (the current one, and that one being rescheduled) > instead of one job to upload those minidumps. So, I'm still not 100% sure this logic is correct. In particular, what will trigger the initial job? You wrote that it would be running "because of old crashes". What's the code that looks for old crashes, and should I make sure to use a wider regex there while making this file naming transition?
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
isherman@chromium.org changed reviewers: + mariakhomenko@chromium.org
+Maria as a reviewer as well. There's still one pending question w.r.t. how this interacts with the WebView uploading logic, but I think at least the Chrome logic is in good shape for being reviewed. Thanks!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool, this lgtm % some minor comments. Thanks Ilya! https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2862353003/diff/100001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:151: mDelegate.migrateMinidumpFilenamesIfNeeded(fileManager); On 2017/05/18 03:04:25, Ilya Sherman wrote: > On 2017/05/16 10:25:49, gsennton wrote: > > On 2017/05/16 07:15:39, Ilya Sherman wrote: > > > On 2017/05/15 13:47:59, gsennton wrote: > > > > Hmm, so the first time we run this Job on a WebView version containing > this > > CL > > > > we will call getMinidumpsReadyForUpload() above - and not retrieve any of > > the > > > > Minidumps from the older WebView versions since those don't include "tryN" > > > > (unless we've tried to upload them already). I guess this is OK as long as > > we > > > > migrate the files before we call the getMinidumpsReadyForUpload() again > > below > > > to > > > > determine whether to reschedule (though we might 'waste' one job on just > > > > migrating old minidumps). This sounds fine given that our priority should > be > > > > uploading minidumps from the new version anyway ;) > > > > > > Yeah, that seems intuitively ok to me, since we do indeed care the most > about > > > new crashes. I suppose there is some concern that we wouldn't run this code > > *at > > > all* if we don't see any crashes from the new version, so the upload of the > > old > > > crashes might be really delayed. Is that the case? Sorry, I definitely > wrote > > > > It depends on what you mean by really delayed - the worst thing that can > happen > > is that we don't see any new crashes. In that case we will still have a job > > running (because of the old crashes), and that job will see that there are no > > new crashes (i.e. getMinidumpsReadyForUpload() returns an empty array), and > thus > > skip through the for-loop above, and then call > > migrateMinidumpFilenamesIfNeeded(). The job will then be rescheduled because > > after the old minidumps are migrated the getMinidumpsReadyForUpload() call > below > > will return a non-empty array. > > The next time the job runs it will upload the migrated files :) > > > > So to sum up: the first time we want to upload minidumps from the old WebView > > version we will need two jobs (the current one, and that one being > rescheduled) > > instead of one job to upload those minidumps. > > So, I'm still not 100% sure this logic is correct. In particular, what will > trigger the initial job? You wrote that it would be running "because of old > crashes". What's the code that looks for old crashes, and should I make sure to > use a wider regex there while making this file naming transition? Right, what I mean by a job being run "because of old crashes" is that a job from a previous version of WebView would be scheduled to execute after WebView has been updated (to include this CL). I.e. the timeline would be: WebView 59 crashes -> An upload job is created and posted to JobScheduler WebView is updated to 60 (or w/e version includes this CL) The upload job created in 59 is scheduled to run - and is handled by the code here (WebView 60). https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:105: // Minidump filenames are migrated within the ProcessInitializationHandler, and hence there Maybe point out that the migration happens as part of appending the logcat - so that it doesn't make sense to move the migration here? (If I stumbled upon this code I would wonder why we wouldn't just move that migration here ;)). https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:506: // logcat output to. Mark these as ready for upload. If there is a fresh minidump Thanks for adding these comments, they're super useful! :) (especially since the way processMinidumpsSansLogcat() works is a bit weird the first time you see it). https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:514: File[] allMinidumpFiles = listCrashFiles(MINIDUMP_READY_FOR_UPLOAD_PATTERN); Technically, we should list both the files ready for upload and the minidumps-sans-logcat here (to catch minidumps from old WebView versions), but I don't think it matters much (all the minidumps will be renamed during upload jobs later on anyway - as you have made sure ;)). https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java:280: public void testGetMinidumpsReadyForUplaod_MultiDigitMaxTries() { typo: testGetMinidumpsReadyForUplaod_MultiDigitMaxTries ^^ https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploaderImplTest.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploaderImplTest.java:110: File uploadedFirstFile = new File(mCrashDir, "firstFile.dmp0.try0"); I guess we're always hitting the if-clause rather than the else-clause here ;) In the else-clause uploadedFirstFile should be named "firstFile.up0.try0", while expectedSecondFile should be named "secondFile.dmp0.try1" meaning that we succeed to upload firstFile, and fail to upload secondFile (in practice we probably always upload secondFile, and fail to upload firstFile, since secondFile was created on disk after firstFile - and secondFile is thus seen as the most recent crash). So please change File uploadedFirstFile = new File(mCrashDir, "firstFile.dmp0.try0"); to File uploadedFirstFile = new File(mCrashDir, "firstFile.up0.try0"); and (on the line after assertTrue(uploadedFirstFile.exists());), change expectedSecondFile = new File(mCrashDir, "secondFile.up0.try0"); to expectedSecondFile = new File(mCrashDir, "secondFile.dmp0.try1"); or we could just remove the else-clause altogether - but then we are assuming that we will always try to upload secondFile first :) (an implementation detail which might change in the future).
https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:173: final File[] newMinidumps = crashFileManager.getMinidumpsMissingLogcat(); I feel somewhat like we should just be liberal in what we transmit to the crash receiver service, because we don't go through the https upload logic here, and there are no retries, so renaming a bunch of files (especially as that duplicates migrateMinidumpFilenamesIfNeeded code) just so that getMinidumpsReadyForUpload seems a bit convoluted. Could we just keep getAllMinidumpFiles (that just returns all files that match *.dmp*) for this case? https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:567: File tmpFile = createMinidumpTmpFile(tmpDir); This determines the file name that crashes are received in the webview service. As it stands the minidump is not uploadable at this point, and it relies on a migration later on. This seems a little weird to me. maybe it would be better if copyMinidumpFromFD named these files correctly directly. Or, maybe it should take a file name template argument so that the name it uses can be controlled somewhat.
https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:173: final File[] newMinidumps = crashFileManager.getMinidumpsMissingLogcat(); On 2017/05/18 15:00:04, Tobias Sargeant wrote: > I feel somewhat like we should just be liberal in what we transmit to the crash > receiver service, because we don't go through the https upload logic here, and > there are no retries, so renaming a bunch of files (especially as that > duplicates migrateMinidumpFilenamesIfNeeded code) just so that > getMinidumpsReadyForUpload seems a bit convoluted. Could we just keep > getAllMinidumpFiles (that just returns all files that match *.dmp*) for this > case? Yeah, good point - I agree with this suggestion ;) https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:567: File tmpFile = createMinidumpTmpFile(tmpDir); On 2017/05/18 15:00:04, Tobias Sargeant wrote: > This determines the file name that crashes are received in the webview service. > As it stands the minidump is not uploadable at this point, and it relies on a > migration later on. This seems a little weird to me. maybe it would be better if > copyMinidumpFromFD named these files correctly directly. Or, maybe it should > take a file name template argument so that the name it uses can be controlled > somewhat. createMinidumpTmpFile does not determine the final file name: createUniqueMinidumpNameForUid() does, and it's being changed in this CL to directly create a correctly named minidump. The only reason we have the migration logic in MinidumpUploaderImpl is to update minidumps from older WebView versions hanging around in the WebView data directory.
https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:567: File tmpFile = createMinidumpTmpFile(tmpDir); On 2017/05/18 15:38:12, gsennton wrote: > On 2017/05/18 15:00:04, Tobias Sargeant wrote: > > This determines the file name that crashes are received in the webview > service. > > As it stands the minidump is not uploadable at this point, and it relies on a > > migration later on. This seems a little weird to me. maybe it would be better > if > > copyMinidumpFromFD named these files correctly directly. Or, maybe it should > > take a file name template argument so that the name it uses can be controlled > > somewhat. > > createMinidumpTmpFile does not determine the final file name: > createUniqueMinidumpNameForUid() does, and it's being changed in this CL to > directly create a correctly named minidump. The only reason we have the > migration logic in MinidumpUploaderImpl is to update minidumps from older > WebView versions hanging around in the WebView data directory. Ok, thanks for the clarification.
LGTM so you're not blocked after addressing Gustav's comments.
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:173: final File[] newMinidumps = crashFileManager.getMinidumpsMissingLogcat(); On 2017/05/18 15:38:12, gsennton wrote: > On 2017/05/18 15:00:04, Tobias Sargeant wrote: > > I feel somewhat like we should just be liberal in what we transmit to the > crash > > receiver service, because we don't go through the https upload logic here, and > > there are no retries, so renaming a bunch of files (especially as that > > duplicates migrateMinidumpFilenamesIfNeeded code) just so that > > getMinidumpsReadyForUpload seems a bit convoluted. Could we just keep > > getAllMinidumpFiles (that just returns all files that match *.dmp*) for this > > case? > > Yeah, good point - I agree with this suggestion ;) I also agree, though now that I better understand the exact data flow, I implemented this perhaps a bit differently than y'all were expecting. If I understand correctly, there was never a reason to have MAX_MINIDUMP_UPLOAD_TRIES in this file, since "foo.dmpPID" is the only filename format we expect to see here. Does that sound right? Sorry for initially making all this way more confusing than it needed to be! I myself was very confused =P https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:105: // Minidump filenames are migrated within the ProcessInitializationHandler, and hence there On 2017/05/18 13:36:26, gsennton wrote: > Maybe point out that the migration happens as part of appending the logcat - so > that it doesn't make sense to move the migration here? (If I stumbled upon this > code I would wonder why we wouldn't just move that migration here ;)). Done. https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2862353003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:506: // logcat output to. Mark these as ready for upload. If there is a fresh minidump On 2017/05/18 13:36:26, gsennton wrote: > Thanks for adding these comments, they're super useful! :) > (especially since the way processMinidumpsSansLogcat() works is a bit weird the > first time you see it). Yeah, I'd love to just have the code be clearer and in less dire need of comments, but I didn't come up with a better way to structure it than this. Glad the comments at least are helpful =) https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:514: File[] allMinidumpFiles = listCrashFiles(MINIDUMP_READY_FOR_UPLOAD_PATTERN); On 2017/05/18 13:36:26, gsennton wrote: > Technically, we should list both the files ready for upload and the > minidumps-sans-logcat here (to catch minidumps from old WebView versions), but I > don't think it matters much (all the minidumps will be renamed during upload > jobs later on anyway - as you have made sure ;)). Yeah, unless you feel strongly otherwise, I'm inclined to leave the code in the "eventually consistent" state for old minidumps rather than carrying forward extra maintenance burden. I figured that worst case, we'd double the number of stored files for a month (until they're aged out); and since the storage restriction was chosen somewhat arbitrarily to begin with, temporarily doubling it doesn't seem too bad. https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java:280: public void testGetMinidumpsReadyForUplaod_MultiDigitMaxTries() { On 2017/05/18 13:36:26, gsennton wrote: > typo: > testGetMinidumpsReadyForUplaod_MultiDigitMaxTries > ^^ Done. https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploaderImplTest.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploaderImplTest.java:110: File uploadedFirstFile = new File(mCrashDir, "firstFile.dmp0.try0"); On 2017/05/18 13:36:26, gsennton wrote: > I guess we're always hitting the if-clause rather than the else-clause here ;) > In the else-clause uploadedFirstFile should be named "firstFile.up0.try0", while > expectedSecondFile should be named "secondFile.dmp0.try1" meaning that we > succeed to upload firstFile, and fail to upload secondFile (in practice we > probably always upload secondFile, and fail to upload firstFile, since > secondFile was created on disk after firstFile - and secondFile is thus seen as > the most recent crash). So please change > File uploadedFirstFile = new File(mCrashDir, "firstFile.dmp0.try0"); > to > File uploadedFirstFile = new File(mCrashDir, "firstFile.up0.try0"); > > and (on the line after assertTrue(uploadedFirstFile.exists());), change > expectedSecondFile = new File(mCrashDir, "secondFile.up0.try0"); > to > expectedSecondFile = new File(mCrashDir, "secondFile.dmp0.try1"); > > or we could just remove the else-clause altogether - but then we are assuming > that we will always try to upload secondFile first :) (an implementation detail > which might change in the future). Whoops, good catch! Fixed.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
still lgtm https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2862353003/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:173: final File[] newMinidumps = crashFileManager.getMinidumpsMissingLogcat(); On 2017/05/18 21:47:18, Ilya Sherman wrote: > On 2017/05/18 15:38:12, gsennton wrote: > > On 2017/05/18 15:00:04, Tobias Sargeant wrote: > > > I feel somewhat like we should just be liberal in what we transmit to the > > crash > > > receiver service, because we don't go through the https upload logic here, > and > > > there are no retries, so renaming a bunch of files (especially as that > > > duplicates migrateMinidumpFilenamesIfNeeded code) just so that > > > getMinidumpsReadyForUpload seems a bit convoluted. Could we just keep > > > getAllMinidumpFiles (that just returns all files that match *.dmp*) for this > > > case? > > > > Yeah, good point - I agree with this suggestion ;) > > I also agree, though now that I better understand the exact data flow, I > implemented this perhaps a bit differently than y'all were expecting. If I > understand correctly, there was never a reason to have MAX_MINIDUMP_UPLOAD_TRIES > in this file, since "foo.dmpPID" is the only filename format we expect to see > here. Does that sound right? Yeah, that's true (earlier though, I think we didn't care enough to make this code different from the rest of the places calling getAllMinidumpFiles()). https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2862353003/diff/140001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:514: File[] allMinidumpFiles = listCrashFiles(MINIDUMP_READY_FOR_UPLOAD_PATTERN); On 2017/05/18 21:47:18, Ilya Sherman wrote: > On 2017/05/18 13:36:26, gsennton wrote: > > Technically, we should list both the files ready for upload and the > > minidumps-sans-logcat here (to catch minidumps from old WebView versions), but > I > > don't think it matters much (all the minidumps will be renamed during upload > > jobs later on anyway - as you have made sure ;)). > > Yeah, unless you feel strongly otherwise, I'm inclined to leave the code in the > "eventually consistent" state for old minidumps rather than carrying forward > extra maintenance burden. I figured that worst case, we'd double the number of > stored files for a month (until they're aged out); and since the storage > restriction was chosen somewhat arbitrarily to begin with, temporarily doubling > it doesn't seem too bad. sgtm
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tobiasjs@chromium.org Link to the patchset: https://codereview.chromium.org/2862353003/#ps180001 (title: "Rebase")
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": 180001, "attempt_start_ts": 1495228095298220,
"parent_rev": "2c8986a0628c40c2c78175723454e0bd5f4b7abc", "commit_rev":
"d4b0825f3059a63da4210b9e8fa1d53271d60b0d"}
Message was sent while issue was closed.
Description was changed from ========== [Crash Reporting] Fix a race between uploading and appending logcat data. Prior to this CL, files ending with ".dmp" could potentially be simulatenously scheduled for appending logcat data and scheduled for upload. This CL resolves the race by requiring a ".try0" suffix to be present prior to attempting an upload. BUG=719129 R=gsennton@chromium.org ========== to ========== [Crash Reporting] Fix a race between uploading and appending logcat data. Prior to this CL, files ending with ".dmp" could potentially be simulatenously scheduled for appending logcat data and scheduled for upload. This CL resolves the race by requiring a ".try0" suffix to be present prior to attempting an upload. BUG=719129 R=gsennton@chromium.org Review-Url: https://codereview.chromium.org/2862353003 Cr-Commit-Position: refs/heads/master@{#473319} Committed: https://chromium.googlesource.com/chromium/src/+/d4b0825f3059a63da4210b9e8fa1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/d4b0825f3059a63da4210b9e8fa1... |
