[Android] The implementation for renaming Minidump files with suffix try0 is incorrect. |
||
Issue description
Steps to repro:
1. Grab a mobile device and go offline (turn off WiFi and Data).
2. Navigate to chrome://crash (to cause a crash and a minidump file to be created).
3. Look at the naming of the minidump file that represents the crash in 2 after Chrome tries to upload the file (the upload will fail because we are offline). If logcat extraction succeeds the file will be named something like "foo.dmpX.try0.try1" where X is some number:
Initial file name:
foo.dmpX
File name after logcat extraction:
foo.dmpX.try0
File name after failed upload attempt
foo.dmpX.try0.try1
The file name after the failed upload attempt should be foo.dmpX.try1 - the incorrectly named file will now not be picked up by further upload attempts because the file name doesn't match the MINIDUMP_PATTERN "\\.dmp([0-9]*)(\\.try[0-9])?\\z"
The reason for this bug seems to be the numTries > 0 check inside the method filenameWithIncrementedAttemptNumber:
@VisibleForTesting
public static String filenameWithIncrementedAttemptNumber(String filename) {
int numTried = readAttemptNumber(filename);
if (numTried > 0) {
int newCount = numTried + 1;
return filename.replace(
UPLOAD_ATTEMPT_DELIMITER + numTried, UPLOAD_ATTEMPT_DELIMITER + newCount);
} else {
return filename + UPLOAD_ATTEMPT_DELIMITER + "1";
}
}
i.e. the filename "foo.dmpX.try0" ->
numTried == 0 ->
return filename + UPLOAD_ATTEMPT_DELIMITER + "1"; // = foo.dmpX.try0.try1
Note that there is a similar bug in the method filenameWithForcedUploadState:
/**
* @return The filename to rename to so as to manually force an upload (including clearing any
* previous upload attempt history).
*/
@VisibleForTesting
protected static String filenameWithForcedUploadState(String filename) {
int numTried = readAttemptNumber(filename);
if (numTried > 0) {
filename = filename.replace(
UPLOAD_ATTEMPT_DELIMITER + numTried, UPLOAD_ATTEMPT_DELIMITER + 0);
}
filename = filename.replace(UPLOAD_SKIPPED_MINIDUMP_SUFFIX, UPLOAD_FORCED_MINIDUMP_SUFFIX);
return filename.replace(NOT_YET_UPLOADED_MINIDUMP_SUFFIX, UPLOAD_FORCED_MINIDUMP_SUFFIX);
}
,
Oct 13 2016
Eh, nvm about filenameWithForcedUploadState - I think that case is fine, i.e. in the case of the filename "foo.dmpX.try0" filenameWithForcedUploadState won't do anything about try0 which is correct in that case.
,
Oct 13 2016
Yeah, I think the code is intended to check for failures, but 0 is returned both on success (.try0 suffix) and on failure (no .tryN) suffix. Sounds like a good thing to fix =)
,
Nov 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea commit c764501f3bc7b6a5494d3b2da3bf13d43ee36bea Author: gsennton <gsennton@chromium.org> Date: Tue Nov 15 17:50:05 2016 Correctly rename minidumps "foo.dmpX.try0" after failed uploads. Before appending a logcat to a minidump the minidump file name has the form "foo.dmpX" (where X is a number). After the logcat has been appended the file name is "foo.dmpX.try0". Failing to upload a file on the form "foo.dmpX.try0" would (before the current change) update the file name "foo.dmpX.try0.try1" because "try0" would be handled as if there wasn't any "tryN" suffix at all. This change fixes the file-name update after a failed upload so that the new name is "foo.dmpX.try1" instead. BUG= 655631 Review-Url: https://codereview.chromium.org/2418843002 Cr-Commit-Position: refs/heads/master@{#432212} [modify] https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java [modify] https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java [modify] https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea/components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java
,
Nov 15 2016
|
||
►
Sign in to add a comment |
||
Comment 1 by gsennton@chromium.org
, Oct 13 2016