New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 655631 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

[Android] The implementation for renaming Minidump files with suffix try0 is incorrect.

Project Member Reported by gsennton@chromium.org, Oct 13 2016

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);
}

 
Summary: [Android] The implementation for renaming Minidump files with suffix try0 is incorrect. (was: [AndroidThe implementation for renaming Minidump files with suffix try0 is incorrect.)
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.
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 =)
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment