Correctly rename minidumps "foo.dmpX.try0" after failed uploads.

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
Committed: https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea
Cr-Commit-Position: refs/heads/master@{#432212}

[gsennton, 2016-10-13 17:15:12]

Description was changed from

==========
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
==========

to

==========
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
==========

[gsennton, 2016-10-13 17:15:12]

gsennton@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2016-10-17 23:55:08]

You didn't mail out this CL, but I'm assuming it's ready for review?  (The
Rietveld codereview system is definitely pretty confusing sometimes -- it's not
a rare mistake!)

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
I think I'd prefer to distinguish a successful read of "0" and no read from
readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?

[gsennton, 2016-10-18 07:41:07]

Yeah, I forgot to publish this, but then I also realized I probably won't be
landing this CL this week (I'm working remotely) since I want to test the final
version of the CL (not only through unit tests) before landing it (and testing
it remotely is a bit of a pain) - other CLs I'm landing are not as scary since
they are fairly straight-forward refactors :)

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/17 23:55:08, Ilya Sherman wrote:
> I think I'd prefer to distinguish a successful read of "0" and no read from
> readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?

Yeah, I think some way of declaring that readAttemptNumber failed would be nice.
I generally don't like just returning negative values since there is chance of
creating bugs when using comparison operators on the return-value. But I'm
probably just being too picky ;).

I'll rewrite it to return -1 on failure.

[Ilya Sherman, 2016-10-18 07:45:51]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/18 07:41:07, gsennton wrote:
> On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > I think I'd prefer to distinguish a successful read of "0" and no read from
> > readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?
> 
> Yeah, I think some way of declaring that readAttemptNumber failed would be
nice.
> I generally don't like just returning negative values since there is chance of
> creating bugs when using comparison operators on the return-value. But I'm
> probably just being too picky ;).
> 
> I'll rewrite it to return -1 on failure.

I guess it's Java, so ... is throwing custom exceptions a thing?  I don't know
my way around Java very well.  I'm open to options other than returning -1,
though =)

[gsennton, 2016-10-18 14:43:17]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/18 07:45:50, Ilya Sherman wrote:
> On 2016/10/18 07:41:07, gsennton wrote:
> > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > I think I'd prefer to distinguish a successful read of "0" and no read
from
> > > readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?
> > 
> > Yeah, I think some way of declaring that readAttemptNumber failed would be
> nice.
> > I generally don't like just returning negative values since there is chance
of
> > creating bugs when using comparison operators on the return-value. But I'm
> > probably just being too picky ;).
> > 
> > I'll rewrite it to return -1 on failure.
> 
> I guess it's Java, so ... is throwing custom exceptions a thing?  I don't know
> my way around Java very well.  I'm open to options other than returning -1,
> though =)

I would definitely prefer throwing a custom exception over returning -1 here.
I'm not sure whether this affects performance in any way we care about?

[Ilya Sherman, 2016-10-18 21:08:59]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/18 14:43:16, gsennton wrote:
> On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > On 2016/10/18 07:41:07, gsennton wrote:
> > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > I think I'd prefer to distinguish a successful read of "0" and no read
> from
> > > > readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?
> > > 
> > > Yeah, I think some way of declaring that readAttemptNumber failed would be
> > nice.
> > > I generally don't like just returning negative values since there is
chance
> of
> > > creating bugs when using comparison operators on the return-value. But I'm
> > > probably just being too picky ;).
> > > 
> > > I'll rewrite it to return -1 on failure.
> > 
> > I guess it's Java, so ... is throwing custom exceptions a thing?  I don't
know
> > my way around Java very well.  I'm open to options other than returning -1,
> > though =)
> 
> I would definitely prefer throwing a custom exception over returning -1 here.
> I'm not sure whether this affects performance in any way we care about?

I would guess that any performance impact shouldn't be large enough to be of
concern for us, but I'd definitely check that with someone more familiar with
Java gotchas and best practices.

[gsennton, 2016-10-26 15:23:19]

gsennton@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[gsennton, 2016-10-26 15:23:19]

Hi Maria, please see the discussion in CrashFileManager.java rather than
directly reviewing the CL :)
(I'll probably change the CL depending on that discussion).

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/18 21:08:59, Ilya Sherman wrote:
> On 2016/10/18 14:43:16, gsennton wrote:
> > On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > > On 2016/10/18 07:41:07, gsennton wrote:
> > > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > > I think I'd prefer to distinguish a successful read of "0" and no read
> > from
> > > > > readAttemptNumber, e.g. by returning -1 when no value was read.  WDYT?
> > > > 
> > > > Yeah, I think some way of declaring that readAttemptNumber failed would
be
> > > nice.
> > > > I generally don't like just returning negative values since there is
> chance
> > of
> > > > creating bugs when using comparison operators on the return-value. But
I'm
> > > > probably just being too picky ;).
> > > > 
> > > > I'll rewrite it to return -1 on failure.
> > > 
> > > I guess it's Java, so ... is throwing custom exceptions a thing?  I don't
> know
> > > my way around Java very well.  I'm open to options other than returning
-1,
> > > though =)
> > 
> > I would definitely prefer throwing a custom exception over returning -1
here.
> > I'm not sure whether this affects performance in any way we care about?
> 
> I would guess that any performance impact shouldn't be large enough to be of
> concern for us, but I'd definitely check that with someone more familiar with
> Java gotchas and best practices.

Hmm, thinking about this a bit more - generally you are supposed to use
exceptions only for exceptional cases - which we wouldn't be doing here. I don't
think it's super bad to return -1 when we can't find a try-number. Adding Maria
in case she has any thoughts :).

[Maria, 2016-10-26 20:57:06]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/26 15:23:19, gsennton wrote:
> On 2016/10/18 21:08:59, Ilya Sherman wrote:
> > On 2016/10/18 14:43:16, gsennton wrote:
> > > On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > > > On 2016/10/18 07:41:07, gsennton wrote:
> > > > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > > > I think I'd prefer to distinguish a successful read of "0" and no
read
> > > from
> > > > > > readAttemptNumber, e.g. by returning -1 when no value was read. 
WDYT?
> > > > > 
> > > > > Yeah, I think some way of declaring that readAttemptNumber failed
would
> be
> > > > nice.
> > > > > I generally don't like just returning negative values since there is
> > chance
> > > of
> > > > > creating bugs when using comparison operators on the return-value. But
> I'm
> > > > > probably just being too picky ;).
> > > > > 
> > > > > I'll rewrite it to return -1 on failure.
> > > > 
> > > > I guess it's Java, so ... is throwing custom exceptions a thing?  I
don't
> > know
> > > > my way around Java very well.  I'm open to options other than returning
> -1,
> > > > though =)
> > > 
> > > I would definitely prefer throwing a custom exception over returning -1
> here.
> > > I'm not sure whether this affects performance in any way we care about?
> > 
> > I would guess that any performance impact shouldn't be large enough to be of
> > concern for us, but I'd definitely check that with someone more familiar
with
> > Java gotchas and best practices.
> 
> Hmm, thinking about this a bit more - generally you are supposed to use
> exceptions only for exceptional cases - which we wouldn't be doing here. I
don't
> think it's super bad to return -1 when we can't find a try-number. Adding
Maria
> in case she has any thoughts :).

I think readAttemptNumber needs to be rewritten. Your regular expression
introduces the expectation that there may be more than 9 tries. However,
readAttemptNumber only works with a single digit after try. If the code is
rewritten to use the same regular expression to match the try number, then the
case where readAttemptNumber didn't work, but regular expression matched above
won't be possible.

[Ilya Sherman, 2016-10-26 22:30:09]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/26 20:57:06, Maria wrote:
> On 2016/10/26 15:23:19, gsennton wrote:
> > On 2016/10/18 21:08:59, Ilya Sherman wrote:
> > > On 2016/10/18 14:43:16, gsennton wrote:
> > > > On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > > > > On 2016/10/18 07:41:07, gsennton wrote:
> > > > > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > > > > I think I'd prefer to distinguish a successful read of "0" and no
> read
> > > > from
> > > > > > > readAttemptNumber, e.g. by returning -1 when no value was read. 
> WDYT?
> > > > > > 
> > > > > > Yeah, I think some way of declaring that readAttemptNumber failed
> would
> > be
> > > > > nice.
> > > > > > I generally don't like just returning negative values since there is
> > > chance
> > > > of
> > > > > > creating bugs when using comparison operators on the return-value.
But
> > I'm
> > > > > > probably just being too picky ;).
> > > > > > 
> > > > > > I'll rewrite it to return -1 on failure.
> > > > > 
> > > > > I guess it's Java, so ... is throwing custom exceptions a thing?  I
> don't
> > > know
> > > > > my way around Java very well.  I'm open to options other than
returning
> > -1,
> > > > > though =)
> > > > 
> > > > I would definitely prefer throwing a custom exception over returning -1
> > here.
> > > > I'm not sure whether this affects performance in any way we care about?
> > > 
> > > I would guess that any performance impact shouldn't be large enough to be
of
> > > concern for us, but I'd definitely check that with someone more familiar
> with
> > > Java gotchas and best practices.
> > 
> > Hmm, thinking about this a bit more - generally you are supposed to use
> > exceptions only for exceptional cases - which we wouldn't be doing here. I
> don't
> > think it's super bad to return -1 when we can't find a try-number. Adding
> Maria
> > in case she has any thoughts :).
> 
> I think readAttemptNumber needs to be rewritten. Your regular expression
> introduces the expectation that there may be more than 9 tries. However,
> readAttemptNumber only works with a single digit after try. If the code is
> rewritten to use the same regular expression to match the try number, then the
> case where readAttemptNumber didn't work, but regular expression matched above
> won't be possible.

Just to clarify: readAttemptNumber is used elsewhere as well, so I think it's
still worthwhile to distinguish, in its return value, whether the read was
successful.  And, once that's done, there's no need for the regex check in the
if stmt -- so my suggestion was to update readAttemptNumber to distinguish
between success and failure, vs. needing the extra regex check here.

And, FWIW, readAttemptNumber should support multi-digit attempts now -- Gustav
recently changed this.  Are you pointing out a bug in the new implementation, or
simply remembering the older code? =)

[Maria, 2016-10-27 01:06:12]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/26 22:30:09, Ilya Sherman wrote:
> On 2016/10/26 20:57:06, Maria wrote:
> > On 2016/10/26 15:23:19, gsennton wrote:
> > > On 2016/10/18 21:08:59, Ilya Sherman wrote:
> > > > On 2016/10/18 14:43:16, gsennton wrote:
> > > > > On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > > > > > On 2016/10/18 07:41:07, gsennton wrote:
> > > > > > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > > > > > I think I'd prefer to distinguish a successful read of "0" and
no
> > read
> > > > > from
> > > > > > > > readAttemptNumber, e.g. by returning -1 when no value was read. 
> > WDYT?
> > > > > > > 
> > > > > > > Yeah, I think some way of declaring that readAttemptNumber failed
> > would
> > > be
> > > > > > nice.
> > > > > > > I generally don't like just returning negative values since there
is
> > > > chance
> > > > > of
> > > > > > > creating bugs when using comparison operators on the return-value.
> But
> > > I'm
> > > > > > > probably just being too picky ;).
> > > > > > > 
> > > > > > > I'll rewrite it to return -1 on failure.
> > > > > > 
> > > > > > I guess it's Java, so ... is throwing custom exceptions a thing?  I
> > don't
> > > > know
> > > > > > my way around Java very well.  I'm open to options other than
> returning
> > > -1,
> > > > > > though =)
> > > > > 
> > > > > I would definitely prefer throwing a custom exception over returning
-1
> > > here.
> > > > > I'm not sure whether this affects performance in any way we care
about?
> > > > 
> > > > I would guess that any performance impact shouldn't be large enough to
be
> of
> > > > concern for us, but I'd definitely check that with someone more familiar
> > with
> > > > Java gotchas and best practices.
> > > 
> > > Hmm, thinking about this a bit more - generally you are supposed to use
> > > exceptions only for exceptional cases - which we wouldn't be doing here. I
> > don't
> > > think it's super bad to return -1 when we can't find a try-number. Adding
> > Maria
> > > in case she has any thoughts :).
> > 
> > I think readAttemptNumber needs to be rewritten. Your regular expression
> > introduces the expectation that there may be more than 9 tries. However,
> > readAttemptNumber only works with a single digit after try. If the code is
> > rewritten to use the same regular expression to match the try number, then
the
> > case where readAttemptNumber didn't work, but regular expression matched
above
> > won't be possible.
> 
> Just to clarify: readAttemptNumber is used elsewhere as well, so I think it's
> still worthwhile to distinguish, in its return value, whether the read was
> successful.  And, once that's done, there's no need for the regex check in the
> if stmt -- so my suggestion was to update readAttemptNumber to distinguish
> between success and failure, vs. needing the extra regex check here.
> 
> And, FWIW, readAttemptNumber should support multi-digit attempts now -- Gustav
> recently changed this.  Are you pointing out a bug in the new implementation,
or
> simply remembering the older code? =)

I am reading readAttemptNumber() implementation in this file -- I guess the CL
hasn't been rebased on top of the new code yet. That's what confused me.

If we add -1 vs 0 differentiation (which I don't mind in theory), then we should
update the callers to handle the new return value. There are definitely
assumptions in the code below that this read is valid. And in this method, we
wouldn't want to increment, but probably need to throw an exception.

[gsennton, 2016-11-09 15:27:08]

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:122:
int numTried = readAttemptNumber(filename);
On 2016/10/27 01:06:12, Maria wrote:
> On 2016/10/26 22:30:09, Ilya Sherman wrote:
> > On 2016/10/26 20:57:06, Maria wrote:
> > > On 2016/10/26 15:23:19, gsennton wrote:
> > > > On 2016/10/18 21:08:59, Ilya Sherman wrote:
> > > > > On 2016/10/18 14:43:16, gsennton wrote:
> > > > > > On 2016/10/18 07:45:50, Ilya Sherman wrote:
> > > > > > > On 2016/10/18 07:41:07, gsennton wrote:
> > > > > > > > On 2016/10/17 23:55:08, Ilya Sherman wrote:
> > > > > > > > > I think I'd prefer to distinguish a successful read of "0" and
> no
> > > read
> > > > > > from
> > > > > > > > > readAttemptNumber, e.g. by returning -1 when no value was
read. 
> > > WDYT?
> > > > > > > > 
> > > > > > > > Yeah, I think some way of declaring that readAttemptNumber
failed
> > > would
> > > > be
> > > > > > > nice.
> > > > > > > > I generally don't like just returning negative values since
there
> is
> > > > > chance
> > > > > > of
> > > > > > > > creating bugs when using comparison operators on the
return-value.
> > But
> > > > I'm
> > > > > > > > probably just being too picky ;).
> > > > > > > > 
> > > > > > > > I'll rewrite it to return -1 on failure.
> > > > > > > 
> > > > > > > I guess it's Java, so ... is throwing custom exceptions a thing? 
I
> > > don't
> > > > > know
> > > > > > > my way around Java very well.  I'm open to options other than
> > returning
> > > > -1,
> > > > > > > though =)
> > > > > > 
> > > > > > I would definitely prefer throwing a custom exception over returning
> -1
> > > > here.
> > > > > > I'm not sure whether this affects performance in any way we care
> about?
> > > > > 
> > > > > I would guess that any performance impact shouldn't be large enough to
> be
> > of
> > > > > concern for us, but I'd definitely check that with someone more
familiar
> > > with
> > > > > Java gotchas and best practices.
> > > > 
> > > > Hmm, thinking about this a bit more - generally you are supposed to use
> > > > exceptions only for exceptional cases - which we wouldn't be doing here.
I
> > > don't
> > > > think it's super bad to return -1 when we can't find a try-number.
Adding
> > > Maria
> > > > in case she has any thoughts :).
> > > 
> > > I think readAttemptNumber needs to be rewritten. Your regular expression
> > > introduces the expectation that there may be more than 9 tries. However,
> > > readAttemptNumber only works with a single digit after try. If the code is
> > > rewritten to use the same regular expression to match the try number, then
> the
> > > case where readAttemptNumber didn't work, but regular expression matched
> above
> > > won't be possible.
> > 
> > Just to clarify: readAttemptNumber is used elsewhere as well, so I think
it's
> > still worthwhile to distinguish, in its return value, whether the read was
> > successful.  And, once that's done, there's no need for the regex check in
the
> > if stmt -- so my suggestion was to update readAttemptNumber to distinguish
> > between success and failure, vs. needing the extra regex check here.
> > 
> > And, FWIW, readAttemptNumber should support multi-digit attempts now --
Gustav
> > recently changed this.  Are you pointing out a bug in the new
implementation,
> or
> > simply remembering the older code? =)
> 
> I am reading readAttemptNumber() implementation in this file -- I guess the CL
> hasn't been rebased on top of the new code yet. That's what confused me.
> 
> If we add -1 vs 0 differentiation (which I don't mind in theory), then we
should
> update the callers to handle the new return value. There are definitely
> assumptions in the code below that this read is valid. And in this method, we
> wouldn't want to increment, but probably need to throw an exception.

I'll just return -1 when we can't find "try". However, I don't think
filenameWithIncrementedAttemptNumber() should throw an exception when we
couldn't find "try" - that just means that we didn't append a logcat to the
minidump in question (IIUC). In the current code that case is just handled by
appending ".try1" to the filename.

[gsennton, 2016-11-09 17:07:40]

Ptal :)

[Ilya Sherman, 2016-11-10 00:46:21]

LGTM, thanks.

https://codereview.chromium.org/2418843002/diff/20001/components/minidump_upl...
File
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:182:
* returns -1 if an attempt-number cannot be parsed from the file-name.
nit: Please indent this line 4 extra spaces.

[gsennton, 2016-11-11 12:55:03]

Thanks Ilya!
Committing...

https://codereview.chromium.org/2418843002/diff/20001/components/minidump_upl...
File
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
(right):

https://codereview.chromium.org/2418843002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:182:
* returns -1 if an attempt-number cannot be parsed from the file-name.
On 2016/11/10 00:46:21, Ilya Sherman wrote:
> nit: Please indent this line 4 extra spaces.

Done.

[gsennton, 2016-11-11 12:55:10]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-11-11 12:55:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2418843002/#ps40001
(title: "Nit: format comment.")

[commit-bot: I haz the power, 2016-11-11 12:55:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 14:16:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 14:16:31]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gsennton, 2016-11-14 20:25:52]

Ah, we ensure that the attempt-number is >= 0 in MinidumpUploadService, I
adjusted the code to amend for new logic.

Ilya, ptal again (MinidumpUploadService.java) to ensure I'm not doing something
stupid there :)

[Ilya Sherman, 2016-11-14 23:05:55]

Still LGTM

https://codereview.chromium.org/2418843002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
(right):

https://codereview.chromium.org/2418843002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:232:
if (tries >= MAX_TRIES_ALLOWED || tries < 0) {
Can we remove the "tries < 0" part of this check?  Or is it possible to get a
value that's less than 0 that doesn't indicate that no attempt was read?

[gsennton, 2016-11-15 10:56:56]

Alright, I'm committing this now - I think we should keep the check in
MinidumpUploadService - it shouldn't do any harm :)

https://codereview.chromium.org/2418843002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
(right):

https://codereview.chromium.org/2418843002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:232:
if (tries >= MAX_TRIES_ALLOWED || tries < 0) {
On 2016/11/14 23:05:54, Ilya Sherman wrote:
> Can we remove the "tries < 0" part of this check?  Or is it possible to get a
> value that's less than 0 that doesn't indicate that no attempt was read?

We *shouldn't* be getting a value less than 0 but is there any harm in keeping
the check? :) (I mean it caught an error in this CL so it might catch a future
error - one could imagine that we in the future introduce some bug in the
readAttemptNumber method that returns a negative value?)

[gsennton, 2016-11-15 10:57:58]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-11-15 10:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2418843002/#ps80001
(title: "Added some tests to ensure CrashFileManager.readAttemptNumber returns
-1 in cases like ".try-X" (e.…")

[commit-bot: I haz the power, 2016-11-15 10:58:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 11:04:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 11:04:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gsennton, 2016-11-15 11:16:03]

Haaah, right, now I need an l g t m for MinidumpUploadService as well.

Maria ptal :).

[Maria, 2016-11-15 17:38:28]

lgtm

[gsennton, 2016-11-15 17:44:32]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2016-11-15 17:45:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 17:50:38]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-15 17:50:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-15 18:02:26]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea
Cr-Commit-Position: refs/heads/master@{#432212}
==========

[commit-bot: I haz the power, 2016-11-15 18:02:28]

Patchset 5 (id:??) landed as
https://crrev.com/c764501f3bc7b6a5494d3b2da3bf13d43ee36bea
Cr-Commit-Position: refs/heads/master@{#432212}