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

Issue 655650 link

Starred by 1 user
Status: Fixed
Owner:
█ gsennton@chromium.org
Last visit > 30 days ago
Closed: Jun 2017
Cc:
isherman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

[Android] Investigate some peculiar Minidump uploading edge-cases.

Project Member Reported by gsennton@chromium.org, Oct 13 2016 
I've been playing around a bit with Chrome's Minidump uploading locally and investigated the code more thoroughly and it seems there are some weird edge-cases (maybe you - Ilya - can shed some light on these :))

1. A forced upload will only be tried once - just after we rename it to have the .forced suffix, shouldn't forced uploads be re-tried if they fail? (MINIDUMP_PATTERN doesn't cover forced files so they will never be picked up at later points where we try to upload all the minidumps).

2. Forced uploads skip the wifi-check (isUploadLimited()), is this intentional?

3. It's super ugly that MinidumpPreparationCallable just appends ".try0" to minidump files instead of having a utility method in CrashFileManager do this IMO :)

4. Shouldn't CrashFileManager.filenameWithForcedUploadState(String filename) operate on the MINIDUMP_PATTERN part of a file-name rather than on the whole file-name? E.g. if the file is named footry2foo.dmp0.try2 then filenameWithForcedUploadState will (theoretically) return the String footry3foo.dmp0.try3

5. getAllUploadedFiles seems incorrect given that an uploaded file can be named "foo.upX.tryY" (where X and Y are numbers) - the upload-pattern:
UPLOADED_MINIDUMP_PATTERN = Pattern.compile("\\.up([0-9]*)\\z");

doesn't capture files with a try-suffix after "upX". Right now this doesn't matter much - the only place getAllUploadedFiles is used is in cleanOutAllNonFreshMinidumpFiles, so the uploaded files should be cleaned up when they grow too old anyway (but it should be fixed :)).



All of this poses another related question:
Maybe we should introduce a suite of tests that tests the entire process of uploading minidumps  - instead of just testing each component separately (I haven't looked much at MinidumpUploadServiceTest yet so I'm not sure how much that tests), i.e. mock all the external dependencies of the entire component - e.g. HttpUrlFactory, (and logcat extraction?) - and introduce tests where we go through entire uploads from start to end - and ensure that all files are correctly cleaned up in the end.
That kind of test should catch bugs like  crbug.com/655631  as well. Not sure whether this would require applying a Whole Lotta Love to the current code though ;)

Comment 1 by isherman@chromium.org, Oct 13 2016

Cc: -gsennton@chromium.org isherman@chromium.org
Owner: gsennton@chromium.org
1. You're right, we should probably retry failed forced uploads.

2. Yes, this is intentional -- sometimes users never have a WiFi connection, and we're assuming the forced upload functionality (a) is going to be rarely used, and (b) allows the user to see what the network connection type on their phone currently is, since it's a synchronous operation.  That said, it might be nice to somehow more clearly warn or prompt the user that they are about to upload the crash report over a metered (typically cellular) connection.

3. I agree.

4. Sure, but this shouldn't come up in practice, so I didn't worry about what I imagined to be extra complexity to enforce this constraint.  If it's not much complexity to enforce, and you want to clean that up, please do =)

5. I think I agree.  I vaguely remember looking into this, and since I didn't fix it, I assume that either (a) I got distracted by other work, or (b) I found some reason why the pattern made sense.  Unfortunately I don't remember right now whether (b) was the case somehow...

And, yes, better tests would be wonderful!

(FWIW, most of this code predates me -- I've only worked with it for the last couple of months or so.  I made some cleanup changes while I was working on it, and agree that there is a lot of opportunity to do more.)

Comment 2 by gsennton@chromium.org, Oct 14 2016 

1. Though if we start retrying forced uploads then we probably want to be even more clear that these can be done over a metered connection :)

4. I just think I will have to ensure we don't start naming Minidumps in an arbitrary way - to ensure we don't put something in there that will start clashing with any of the renaming code. I assume for Chrome the Minidumps are just named using a limited set of characters - hex?

I'll prioritize doing the componentization but will happily take a look at these when I have time :)
(and I realize you didn't do all of this - you seem to know the answers to my questions here though so that's super helpful :)).

Comment 3 by isherman@chromium.org, Oct 14 2016 

1. Yes, good point.  That's probably reason enough not to retry the uploads for now -- the user can always tap the link again if need be.

4. I think it's basically lower-alphanum.  Here's an example filename that I see: "chromium-renderer-minidump-b751f6a0ff4bb0ed".  But, I'm not super confident that I understand how these files are named -- I haven't looked at that part of the code closely.

Comment 4 by gsennton@chromium.org, Apr 17 2017 

Update:

1. This has been fixed (IIRC this was fixed by you Ilya ;), anyway - the code seems correct now).
2. Intentional (as per comment #1).
3. This has moved to MinidumpLogcatPrepender - I'm not sure we care enough about this change it.
4. there are several places in CrashFileManager where just filename.replace() some file-suffix with another - so if the actual filename contains any of our keywords it will be renamed incorrectly.
5. This was fixed by Ilya in https://codereview.chromium.org/2751903002

Comment 5 by gsennton@chromium.org, Jun 15 2017 

Marking this as Fixed - I don't think I will look at any of the remaining issues (if we can even call them "issues" :)).

Comment 6 by gsennton@chromium.org, Jun 20 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment