Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737263006:
[Android Crash Reporting] Allow uploading minidumps via the JobScheduler (Closed)
|
Created:
3 years, 9 months ago by Ilya Sherman Modified:
3 years, 9 months ago CC:
chromium-reviews, kalyank, srahim+watch_chromium.org, sadrul, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android Crash Reporting] Allow uploading minidumps via the JobScheduler
The new functionality is guarded behind a Feature.
BUG=694884
R=gsennton@chromium.org, mariakhomenko@chromium.org
Review-Url: https://codereview.chromium.org/2737263006
Cr-Commit-Position: refs/heads/master@{#457584}
Committed: https://chromium.googlesource.com/chromium/src/+/8a5492dc16ac148bde50b46ca756f95a655fad58
Patch Set 1 #Patch Set 2 : Fully implemented, still needs tests #
Total comments: 34
Patch Set 3 : Use shared prefs #
Total comments: 31
Patch Set 4 : Use background_task_scheduler; update LogcatExtractionRunnable tests #Patch Set 5 : Rebase #Patch Set 6 : MinidumpUploadService tests #
Total comments: 4
Patch Set 7 : Restrict to Android M+ #Patch Set 8 : Remove some temporary debug code #Patch Set 9 : Suppress irrelevant FindBugs warnings for test changes #
Total comments: 14
Patch Set 10 : Assert that job scheduled successfully #Messages
Total messages: 57 (32 generated)
Hi Gustav and Maria, PTAL. I haven't worked out the tests yet; but I've tested manually (at least, I've tested the new code path... I should still go back and make sure the old code path still works too). https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:42: // here... Gustav, WDYT? (In either case, not a change I'd want to implement in this CL.) https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:328: PrivacyPreferencesManager.getInstance().enablePotentialCrashUploading(); This move isn't a functionality change; I just moved the code closer to the comment that describes it. https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? Maria and Gustav, WDYT? Do either of you know how we could check whether Chrome is running? https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:78: // TODO(isherman): We never read mRunningJob -- is it obsolete? Gustav, I forget whether we've discussed this before. Does mRunningJob still make sense? It doesn't seem to be read (outside of the assertion below, I suppose...) https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:123: // TODO(isherman): Log success/failure metrics for Chrome. I'm actually not sure how to do this. The upload job might run after the Chrome process has terminated. It seems overly expensive to spin up a bunch of Chrome's startup code only to record metrics. An alternate idea that I had is to record metrics about the pending crash reports at startup -- how many are failed, how many uploaded successfully, etc. It doesn't give us quite the same fine-grained view, but it's in roughly the same spirit, and is far more straightforward to implement. WDYT?
https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:42: // here... On 2017/03/13 03:12:09, Ilya Sherman wrote: > Gustav, WDYT? (In either case, not a change I'd want to implement in this CL.) the directory structure is cache/WebView_Crashes/Crash Reports/ (to fix this we would have to change CrashFileManager to not use "Crash Reports/" directory - otherwise Monochrome and WebView will be using the same directory to store its minidumps). IMO this is not worth fixing. https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:812: android:exported="false"/> Note that the WebView-services are running in their own process (separate from Monochrome), so this service is different in the sense that it runs in the same process as the browser-side of Monochrome/Chrome. (not sure if it matters much in this case). https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? On 2017/03/13 03:12:09, Ilya Sherman wrote: > Maria and Gustav, WDYT? Do either of you know how we could check whether Chrome > is running? When implementing the usage-and-diagnostics check for WebView there was a request for us (WebView) to ensure there was user-consent at the time of the upload (since that could be long after the jobservice job is scheduled. I would guess we should do the same in Chrome. I don't know if there's a way to check whether Chrome is running. It looks like PrivacyPreferencesManager is using a Shared Preference to store the value of the consent (PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) though, could you just read that value? (if you are still running in the browser process I see no harm in doing so). https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java:132: Log.i(TAG, "Succeeded extracting logcat to %s.", fileToUpload.getName()); Do you mean to add this log-statement? (I don't mind - just wanted to point it out in case you missed it). https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:110: JobInfo.NETWORK_TYPE_ANY, permissions); Not sure I like the mismatch between the network type here, and the isNetworkAvailable in the ChromeDelegate. A failure from isNetworkAvailable means increasing the try-number of a minidump by 1, right? So we could easily end up never uploading any minidumps because we run all our tries on a data connection? https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:295: assert !MinidumpUploadService.shouldUseJobSchedulerForUploads(); Should we add this assert in tryUploadAllCrashDumps (or w/e it is called) as well? https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:52: Pattern.compile("\\.(dmp|forced)([0-9]*)(\\.try([0-9]+))?\\z"); Will this change any behaviour in the old implementation? E.g were we retrying forced uploads before this change? https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadCallable.java (left): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadCallable.java:118: assert mPermManager.isMetricsUploadPermitted(); Ideally, I would probably remove this in a separate CL, but if this is urgent I won't block ;) https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:28: private static final int MINIDUMP_UPLOADING_JOB_ID = 42; I think we want different numbers for Chrome and WebView here - otherwise (Mono)Chrome and (Monochrome-)WebView will interrupt each others' jobs. https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:78: // TODO(isherman): We never read mRunningJob -- is it obsolete? On 2017/03/13 03:12:09, Ilya Sherman wrote: > Gustav, I forget whether we've discussed this before. Does mRunningJob still > make sense? It doesn't seem to be read (outside of the assertion below, I > suppose...) It's asserting that we aren't running several jobs at the same time (only on debug-builds though since we are using an assertion). It's just a sanity check. https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:123: // TODO(isherman): Log success/failure metrics for Chrome. On 2017/03/13 03:12:09, Ilya Sherman wrote: > I'm actually not sure how to do this. The upload job might run after the Chrome > process has terminated. It seems overly expensive to spin up a bunch of > Chrome's startup code only to record metrics. > > An alternate idea that I had is to record metrics about the pending crash > reports at startup -- how many are failed, how many uploaded successfully, etc. > It doesn't give us quite the same fine-grained view, but it's in roughly the > same spirit, and is far more straightforward to implement. WDYT? Sounds reasonable (but I don't know much about UMA).
Thanks for the review! I've now updated the CL to resolve the outstanding concerns. I still need to manually test the old code path, and to update tests (currently getting a linker error... upon running the tests -- something to do with JNI registration, I think). https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:42: // here... On 2017/03/13 17:57:17, gsennton wrote: > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > Gustav, WDYT? (In either case, not a change I'd want to implement in this > CL.) > > the directory structure is > cache/WebView_Crashes/Crash Reports/ > > (to fix this we would have to change CrashFileManager to not use "Crash > Reports/" directory - otherwise Monochrome and WebView will be using the same > directory to store its minidumps). > IMO this is not worth fixing. Ah, okay, this makes more sense now. I renamed things a bit to hopefully clarify the structure. https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:812: android:exported="false"/> On 2017/03/13 17:57:17, gsennton wrote: > Note that the WebView-services are running in their own process (separate from > Monochrome), so this service is different in the sense that it runs in the same > process as the browser-side of Monochrome/Chrome. (not sure if it matters much > in this case). Yeah, I did notice that. I'm not familiar enough with the Android environment to know whether there are any things that I should change here as a result, though. Was there something you were thinking that might need to be different? https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? On 2017/03/13 17:57:17, gsennton wrote: > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > Maria and Gustav, WDYT? Do either of you know how we could check whether > Chrome > > is running? > > When implementing the usage-and-diagnostics check for WebView there was a > request for us (WebView) to ensure there was user-consent at the time of the > upload (since that could be long after the jobservice job is scheduled. > I would guess we should do the same in Chrome. > > I don't know if there's a way to check whether Chrome is running. > > > It looks like PrivacyPreferencesManager is using a Shared Preference to store > the value of the consent > (PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) though, > could you just read that value? (if you are still running in the browser process > I see no harm in doing so). Okay, it does seem like it's safe to directly call PrivacyPreferencesManager.isUsageAndCrashReprotingPermittedByUser() -- whew! I was worried because I saw that sometimes the PrivacyPreferencesManager was instantiated twice, which prevented me from using e.g. PrivacyPreferencesManager.isClientInMetricsSample(), when following these steps: 1. Launch Chrome 2. Navigate to chrome://crash 3. Wait for the minidump upload to be scheduled 4. Force quit Chrome 5. Wait for the upload to run. I'm actually still not entirely sure why the PrivacyPreferencesManager is being created twice, since the return value of getInstance() is cached in a global singleton. I'm therefore still a bit worried that there might be some subtle issue around calling into the PrivacyPreferencesManager from this job. Thoughts? https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnable.java:132: Log.i(TAG, "Succeeded extracting logcat to %s.", fileToUpload.getName()); On 2017/03/13 17:57:17, gsennton wrote: > Do you mean to add this log-statement? (I don't mind - just wanted to point it > out in case you missed it). Yep, it's intentional. I realized as I was debugging this CL that some more log statements would be pretty useful =) https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:110: JobInfo.NETWORK_TYPE_ANY, permissions); On 2017/03/13 17:57:17, gsennton wrote: > Not sure I like the mismatch between the network type here, and the > isNetworkAvailable in the ChromeDelegate. A failure from isNetworkAvailable > means increasing the try-number of a minidump by 1, right? So we could easily > end up never uploading any minidumps because we run all our tries on a data > connection? I believe that matches what we currently do in Chrome. At least this way we have exponential backoff -- already an improvement! =) (There's no way, afaict, to schedule the job to run on "WiFi or Ethernet", metered or not, which is what Chrome currently expects. For M59, I'm planning to update the scheduling constraints to simply be unmetered networks.) https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:295: assert !MinidumpUploadService.shouldUseJobSchedulerForUploads(); On 2017/03/13 17:57:17, gsennton wrote: > Should we add this assert in tryUploadAllCrashDumps (or w/e it is called) as > well? Done. tryUploadAllCrashDumps calls this method internally, but I suppose the extra clarity doesn't hurt =) https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:52: Pattern.compile("\\.(dmp|forced)([0-9]*)(\\.try([0-9]+))?\\z"); On 2017/03/13 17:57:17, gsennton wrote: > Will this change any behaviour in the old implementation? > E.g were we retrying forced uploads before this change? This does correct a bug in the old implementation, yes. AFAICT it is strictly an improvement, though. https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadCallable.java (left): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadCallable.java:118: assert mPermManager.isMetricsUploadPermitted(); On 2017/03/13 17:57:17, gsennton wrote: > Ideally, I would probably remove this in a separate CL, but if this is urgent I > won't block ;) Done. https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:28: private static final int MINIDUMP_UPLOADING_JOB_ID = 42; On 2017/03/13 17:57:17, gsennton wrote: > I think we want different numbers for Chrome and WebView here - otherwise > (Mono)Chrome and (Monochrome-)WebView will interrupt each others' jobs. Done. Good point -- I completely forgot about Monochrome! https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:78: // TODO(isherman): We never read mRunningJob -- is it obsolete? On 2017/03/13 17:57:17, gsennton wrote: > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > Gustav, I forget whether we've discussed this before. Does mRunningJob still > > make sense? It doesn't seem to be read (outside of the assertion below, I > > suppose...) > > It's asserting that we aren't running several jobs at the same time (only on > debug-builds though since we are using an assertion). It's just a sanity check. Okay, as long as it's intended just a sanity-check, that's ok. (Though, I don't think this code is actually reachable in debug builds, at least not without jumping through a lot of hoops -- crash reporting tends to be disabled in debug builds. At least, that's true for Chrome -- I'm less sure about Webview =]) https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:123: // TODO(isherman): Log success/failure metrics for Chrome. On 2017/03/13 17:57:17, gsennton wrote: > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > I'm actually not sure how to do this. The upload job might run after the > Chrome > > process has terminated. It seems overly expensive to spin up a bunch of > > Chrome's startup code only to record metrics. > > > > An alternate idea that I had is to record metrics about the pending crash > > reports at startup -- how many are failed, how many uploaded successfully, > etc. > > It doesn't give us quite the same fine-grained view, but it's in roughly the > > same spirit, and is far more straightforward to implement. WDYT? > > Sounds reasonable (but I don't know much about UMA). Actually, now that I realize that it's okay to use the shared prefs from the job, I think it's simpler to just log success/failure metrics as we did before.
https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:55: // suffix... By the way, Gustav, I noticed that uploaded files didn't seem to be being deleted by the cleanup code... and found what I think might be a bug: The pattern for uploaded files doesn't match the actual uploaded filenames. I'm guessing this is probably pretty important to fix for Webview for M58, yeah?
https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? On 2017/03/14 02:18:55, Ilya Sherman wrote: > On 2017/03/13 17:57:17, gsennton wrote: > > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > > Maria and Gustav, WDYT? Do either of you know how we could check whether > > Chrome > > > is running? > > > > When implementing the usage-and-diagnostics check for WebView there was a > > request for us (WebView) to ensure there was user-consent at the time of the > > upload (since that could be long after the jobservice job is scheduled. > > I would guess we should do the same in Chrome. > > > > I don't know if there's a way to check whether Chrome is running. > > > > > > It looks like PrivacyPreferencesManager is using a Shared Preference to store > > the value of the consent > > (PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) though, > > could you just read that value? (if you are still running in the browser > process > > I see no harm in doing so). > > Okay, it does seem like it's safe to directly call > PrivacyPreferencesManager.isUsageAndCrashReprotingPermittedByUser() -- whew! I > was worried because I saw that sometimes the PrivacyPreferencesManager was > instantiated twice, which prevented me from using e.g. > PrivacyPreferencesManager.isClientInMetricsSample(), when following these steps: > > 1. Launch Chrome > 2. Navigate to chrome://crash > 3. Wait for the minidump upload to be scheduled > 4. Force quit Chrome > 5. Wait for the upload to run. > > I'm actually still not entirely sure why the PrivacyPreferencesManager is being > created twice, since the return value of getInstance() is cached in a global > singleton. I'm therefore still a bit worried that there might be some subtle > issue around calling into the PrivacyPreferencesManager from this job. > Thoughts? When is PrivacyPreferencesManager being created (twice)? I would assume that when you force-quit Chrome the browser process is killed and when the upload-job is later run the browser process would be re-started (thus again initializing PrivacyPreferencesManager if you are using that class inside the uploading-code). https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:110: JobInfo.NETWORK_TYPE_ANY, permissions); On 2017/03/14 02:18:55, Ilya Sherman wrote: > On 2017/03/13 17:57:17, gsennton wrote: > > Not sure I like the mismatch between the network type here, and the > > isNetworkAvailable in the ChromeDelegate. A failure from isNetworkAvailable > > means increasing the try-number of a minidump by 1, right? So we could easily > > end up never uploading any minidumps because we run all our tries on a data > > connection? > > I believe that matches what we currently do in Chrome. At least this way we > have exponential backoff -- already an improvement! =) > > (There's no way, afaict, to schedule the job to run on "WiFi or Ethernet", > metered or not, which is what Chrome currently expects. For M59, I'm planning > to update the scheduling constraints to simply be unmetered networks.) Right, because updating to only using unmetered networks is too scary of a change for 58? https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:78: // TODO(isherman): We never read mRunningJob -- is it obsolete? On 2017/03/14 02:18:55, Ilya Sherman wrote: > On 2017/03/13 17:57:17, gsennton wrote: > > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > > Gustav, I forget whether we've discussed this before. Does mRunningJob > still > > > make sense? It doesn't seem to be read (outside of the assertion below, I > > > suppose...) > > > > It's asserting that we aren't running several jobs at the same time (only on > > debug-builds though since we are using an assertion). It's just a sanity > check. > > Okay, as long as it's intended just a sanity-check, that's ok. (Though, I don't > think this code is actually reachable in debug builds, at least not without > jumping through a lot of hoops -- crash reporting tends to be disabled in debug > builds. At least, that's true for Chrome -- I'm less sure about Webview =]) Yeah, it's only enabled for WebView with a certain command-line flag turned on (something like --enable-crash-uploading). https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:123: // TODO(isherman): Log success/failure metrics for Chrome. On 2017/03/14 02:18:55, Ilya Sherman wrote: > On 2017/03/13 17:57:17, gsennton wrote: > > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > > I'm actually not sure how to do this. The upload job might run after the > > Chrome > > > process has terminated. It seems overly expensive to spin up a bunch of > > > Chrome's startup code only to record metrics. > > > > > > An alternate idea that I had is to record metrics about the pending crash > > > reports at startup -- how many are failed, how many uploaded successfully, > > etc. > > > It doesn't give us quite the same fine-grained view, but it's in roughly the > > > same spirit, and is far more straightforward to implement. WDYT? > > > > Sounds reasonable (but I don't know much about UMA). > > Actually, now that I realize that it's okay to use the shared prefs from the > job, I think it's simpler to just log success/failure metrics as we did before. It's fine as long as we do it just from the browser process I think (i.e. as long as we don't create a separate process for uploading minidumps). SharedPrefs are thread-safe but not process-safe (I think). https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:75: public boolean isMetricsUploadPermitted() { Why did you add this back? https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38: */ Maybe add a comment about this having to be different to Chrome's job-ids as well because of Monochrome? https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:54: private static final int MINIDUMP_UPLOADING_JOB_ID = 142; I just realized it might be good to look for the IDs we are already using ;). It looks like the different values for JobIds in Chrome are being set in components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java I think we should at least add a comment there describing which IDs we are using for WebView+Chrome Minidump uploading. (in the long run we might want to just use all the utility in components/background_task_scheduler/ to implement our job-scheduler). https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:267: getCrashType(getNewNameAfterSuccessfulUpload(fileName))); Hiding this call in here seems like an implementation detail that could easily be missed/overseen? (I might be wrong though ;)) https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:55: // suffix... On 2017/03/14 02:22:03, Ilya Sherman wrote: > By the way, Gustav, I noticed that uploaded files didn't seem to be being > deleted by the cleanup code... and found what I think might be a bug: The > pattern for uploaded files doesn't match the actual uploaded filenames. I'm > guessing this is probably pretty important to fix for Webview for M58, yeah? Huh, that would indeed be important to fix for 58 :) (and to get it into some part of the Beta). Could you please file a bug? I can try to take a look tomorrow (but after that I'll be OOO for the rest of the week :/). https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:196: * returns -1 if an attempt-number cannot be parsed from the file-name. Update comment please ;) https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:54: .setBackoffCriteria(JOB_INITIAL_BACKOFF_TIME_IN_MS, JOB_BACKOFF_POLICY) Maybe assert that we haven't set a back-off criteria already?
I looked through everything and don't have any real comments of value (just style nits). It seems like most of the original comments have been resolved with Gustav. Let me know if I missed any unresolved issues (I did comment on the reading from preference manager part). https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > On 2017/03/14 02:18:55, Ilya Sherman wrote: > > On 2017/03/13 17:57:17, gsennton wrote: > > > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > > > Maria and Gustav, WDYT? Do either of you know how we could check whether > > > Chrome > > > > is running? > > > > > > When implementing the usage-and-diagnostics check for WebView there was a > > > request for us (WebView) to ensure there was user-consent at the time of the > > > upload (since that could be long after the jobservice job is scheduled. > > > I would guess we should do the same in Chrome. > > > > > > I don't know if there's a way to check whether Chrome is running. > > > > > > > > > It looks like PrivacyPreferencesManager is using a Shared Preference to > store > > > the value of the consent > > > (PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) > though, > > > could you just read that value? (if you are still running in the browser > > process > > > I see no harm in doing so). > > > > Okay, it does seem like it's safe to directly call > > PrivacyPreferencesManager.isUsageAndCrashReprotingPermittedByUser() -- whew! > I > > was worried because I saw that sometimes the PrivacyPreferencesManager was > > instantiated twice, which prevented me from using e.g. > > PrivacyPreferencesManager.isClientInMetricsSample(), when following these > steps: > > > > 1. Launch Chrome > > 2. Navigate to chrome://crash > > 3. Wait for the minidump upload to be scheduled > > 4. Force quit Chrome > > 5. Wait for the upload to run. > > > > I'm actually still not entirely sure why the PrivacyPreferencesManager is > being > > created twice, since the return value of getInstance() is cached in a global > > singleton. I'm therefore still a bit worried that there might be some subtle > > issue around calling into the PrivacyPreferencesManager from this job. > > Thoughts? > > When is PrivacyPreferencesManager being created (twice)? I would assume that > when you force-quit Chrome the browser process is killed and when the upload-job > is later run the browser process would be re-started (thus again initializing > PrivacyPreferencesManager if you are using that class inside the > uploading-code). I went and read the documentation. Jobs do run within the application's process (browser process for us). Here's what I would expect to happen: 1. Launch Chrome. PreferenceManager gets created as an in-memory static 2. chrome://crash will crash a renderer, browser process is alive 3. minidump upload is scheduled using the same preference manager value to set isPermittedByUser 4. Force quit chrome. Browser process is dead. Bye bye everything. 5. Upload is scheduled and run. Browser process is started. PreferenceManager is created. If ChromeTabbedActivity were started, it would use the same object. So if you see something different, please comment. https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:71: } nit: I think there should still be newlines between each method implementation here https://codereview.chromium.org/2737263006/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:177: // TODO(isherman): Write tests for both feature flag states. Is this todo for before submitting this change? https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:210: public static int readAttemptNumberInternal(String filename) { can this be package-visible instead? https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("couldn't schedule " + uploadJob); nit: capitalize couldn't https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:92: Log.i(TAG, "Attempting to upload %d minidumps ", minidumps.length); nit: no space needed at the end of this string
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
isherman@chromium.org changed reviewers: + tobiasjs@chromium.org
Thank you both for your reviews! I'm adding Toby for OWNERS review of the android_webview changes. This CL is not 100% complete -- I still need to update the MinidumpUploadServiceTests. However, other than that, I think it's in good shape: I believe I've addressed all the other outstanding comments, and have manually tested both the old and the new code paths. https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:93: // periodically. How hard would that be to implement? On 2017/03/14 19:03:32, Maria wrote: > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > On 2017/03/14 02:18:55, Ilya Sherman wrote: > > > On 2017/03/13 17:57:17, gsennton wrote: > > > > On 2017/03/13 03:12:09, Ilya Sherman wrote: > > > > > Maria and Gustav, WDYT? Do either of you know how we could check > whether > > > > Chrome > > > > > is running? > > > > > > > > When implementing the usage-and-diagnostics check for WebView there was a > > > > request for us (WebView) to ensure there was user-consent at the time of > the > > > > upload (since that could be long after the jobservice job is scheduled. > > > > I would guess we should do the same in Chrome. > > > > > > > > I don't know if there's a way to check whether Chrome is running. > > > > > > > > > > > > It looks like PrivacyPreferencesManager is using a Shared Preference to > > store > > > > the value of the consent > > > > (PrivacyPreferencesManager.isUsageAndCrashReportingPermittedByUser()) > > though, > > > > could you just read that value? (if you are still running in the browser > > > process > > > > I see no harm in doing so). > > > > > > Okay, it does seem like it's safe to directly call > > > PrivacyPreferencesManager.isUsageAndCrashReprotingPermittedByUser() -- whew! > > > I > > > was worried because I saw that sometimes the PrivacyPreferencesManager was > > > instantiated twice, which prevented me from using e.g. > > > PrivacyPreferencesManager.isClientInMetricsSample(), when following these > > steps: > > > > > > 1. Launch Chrome > > > 2. Navigate to chrome://crash > > > 3. Wait for the minidump upload to be scheduled > > > 4. Force quit Chrome > > > 5. Wait for the upload to run. > > > > > > I'm actually still not entirely sure why the PrivacyPreferencesManager is > > being > > > created twice, since the return value of getInstance() is cached in a global > > > singleton. I'm therefore still a bit worried that there might be some > subtle > > > issue around calling into the PrivacyPreferencesManager from this job. > > > Thoughts? > > > > When is PrivacyPreferencesManager being created (twice)? I would assume that > > when you force-quit Chrome the browser process is killed and when the > upload-job > > is later run the browser process would be re-started (thus again initializing > > PrivacyPreferencesManager if you are using that class inside the > > uploading-code). > > I went and read the documentation. Jobs do run within the application's process > (browser process for us). > > Here's what I would expect to happen: > 1. Launch Chrome. PreferenceManager gets created as an in-memory static > 2. chrome://crash will crash a renderer, browser process is alive > 3. minidump upload is scheduled using the same preference manager value to set > isPermittedByUser > 4. Force quit chrome. Browser process is dead. Bye bye everything. > 5. Upload is scheduled and run. Browser process is started. PreferenceManager is > created. If ChromeTabbedActivity were started, it would use the same object. > > So if you see something different, please comment. I think you're both correct, and that makes a lot of sense. Thank you! I feel much less wary of this code now =) I'm still not really used to the Android process model where processes can be killed and started again, with different bits of initialization code running depending on ... stuff =P https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:110: JobInfo.NETWORK_TYPE_ANY, permissions); On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > On 2017/03/14 02:18:55, Ilya Sherman wrote: > > On 2017/03/13 17:57:17, gsennton wrote: > > > Not sure I like the mismatch between the network type here, and the > > > isNetworkAvailable in the ChromeDelegate. A failure from isNetworkAvailable > > > means increasing the try-number of a minidump by 1, right? So we could > easily > > > end up never uploading any minidumps because we run all our tries on a data > > > connection? > > > > I believe that matches what we currently do in Chrome. At least this way we > > have exponential backoff -- already an improvement! =) > > > > (There's no way, afaict, to schedule the job to run on "WiFi or Ethernet", > > metered or not, which is what Chrome currently expects. For M59, I'm planning > > to update the scheduling constraints to simply be unmetered networks.) > > Right, because updating to only using unmetered networks is too scary of a > change for 58? Affirmative =) https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:75: public boolean isMetricsUploadPermitted() { On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > Why did you add this back? Mmm, didn't you ask me to? Maybe I misunderstood =P This comment: https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38: */ On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > Maybe add a comment about this having to be different to Chrome's job-ids as > well because of Monochrome? Acknowledged. https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java:71: } On 2017/03/14 19:03:32, Maria wrote: > nit: I think there should still be newlines between each method implementation > here Done. https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:54: private static final int MINIDUMP_UPLOADING_JOB_ID = 142; On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > I just realized it might be good to look for the IDs we are already using ;). > It looks like the different values for JobIds in Chrome are being set in > components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java > > I think we should at least add a comment there describing which IDs we are using > for WebView+Chrome Minidump uploading. > (in the long run we might want to just use all the utility in > components/background_task_scheduler/ to implement our job-scheduler). Good call! Moved the constants into that component. And, yeah, I think after merging things to M58 I'll update the code to use that component. Looks much nicer than maintaining two similar-but-not-the-same code paths =D https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:267: getCrashType(getNewNameAfterSuccessfulUpload(fileName))); On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > Hiding this call in here seems like an implementation detail that could easily > be missed/overseen? (I might be wrong though ;)) That's a very good point, and I actually had a bug for incrementing the failure count. The current state is awkward because MinidumpUploadCallable renames successful crashes, but not failed ones. I added thorough documentation, though, in hopes that it will help (a) clarify and (b) alert the reader to be careful. Do you think that's sufficient for now? https://codereview.chromium.org/2737263006/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:177: // TODO(isherman): Write tests for both feature flag states. On 2017/03/14 19:03:32, Maria wrote: > Is this todo for before submitting this change? Yep. Done. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:55: // suffix... On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > On 2017/03/14 02:22:03, Ilya Sherman wrote: > > By the way, Gustav, I noticed that uploaded files didn't seem to be being > > deleted by the cleanup code... and found what I think might be a bug: The > > pattern for uploaded files doesn't match the actual uploaded filenames. I'm > > guessing this is probably pretty important to fix for Webview for M58, yeah? > > Huh, that would indeed be important to fix for 58 :) (and to get it into some > part of the Beta). > Could you please file a bug? > I can try to take a look tomorrow (but after that I'll be OOO for the rest of > the week :/). One better: I sent you a CL =) https://codereview.chromium.org/2751903002/ https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:196: * returns -1 if an attempt-number cannot be parsed from the file-name. On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > Update comment please ;) Done. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:210: public static int readAttemptNumberInternal(String filename) { On 2017/03/14 19:03:32, Maria wrote: > can this be package-visible instead? Done. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:54: .setBackoffCriteria(JOB_INITIAL_BACKOFF_TIME_IN_MS, JOB_BACKOFF_POLICY) On 2017/03/14 18:17:29, gsennton_OOO_Mar16-17 wrote: > Maybe assert that we haven't set a back-off criteria already? I think it's a good idea, but I'm not seeing any way in the API to read the currently set value, so I'm not sure how to do this. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("couldn't schedule " + uploadJob); On 2017/03/14 19:03:32, Maria wrote: > nit: capitalize couldn't Done. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:92: Log.i(TAG, "Attempting to upload %d minidumps ", minidumps.length); On 2017/03/14 19:03:32, Maria wrote: > nit: no space needed at the end of this string Done.
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
On 2017/03/15 02:13:35, Ilya Sherman wrote: > This CL is not 100% complete -- I still need to update the > MinidumpUploadServiceTests. However, other than that, I think it's in good > shape: I believe I've addressed all the other outstanding comments, and have > manually tested both the old and the new code paths. I've now updated the MinidumpUploadServiceTests as well. PTAL =)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % some minor comments. I won't be able to look at this any more this week (I don't know whether I'll have internet-access while OOO, either way, I'm not bringing a laptop), if you have any questions you can try to ping me. https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2737263006/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:75: public boolean isMetricsUploadPermitted() { On 2017/03/15 02:13:34, Ilya Sherman wrote: > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > Why did you add this back? > > Mmm, didn't you ask me to? Maybe I misunderstood =P This comment: > https://codereview.chromium.org/2737263006/diff/20001/components/minidump_upl... Oh, haha, that's right ;). (I find navigating codereviews a bit of a pain - sometimes I'd like to see a view of three patch sets instead of two - to be able to match changes from PS1 -> PS2 -> PS3). https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java (right): https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:54: private static final int MINIDUMP_UPLOADING_JOB_ID = 142; On 2017/03/15 02:13:34, Ilya Sherman wrote: > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > I just realized it might be good to look for the IDs we are already using ;). > > It looks like the different values for JobIds in Chrome are being set in > > > components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java > > > > I think we should at least add a comment there describing which IDs we are > using > > for WebView+Chrome Minidump uploading. > > (in the long run we might want to just use all the utility in > > components/background_task_scheduler/ to implement our job-scheduler). > > Good call! Moved the constants into that component. > > And, yeah, I think after merging things to M58 I'll update the code to use that > component. Looks much nicer than maintaining two similar-but-not-the-same code > paths =D Indeed, I wonder if there will be anything stopping us from using that code-path for WebView as well. Will have to take a look ;) https://codereview.chromium.org/2737263006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java:267: getCrashType(getNewNameAfterSuccessfulUpload(fileName))); On 2017/03/15 02:13:34, Ilya Sherman wrote: > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > Hiding this call in here seems like an implementation detail that could easily > > be missed/overseen? (I might be wrong though ;)) > > That's a very good point, and I actually had a bug for incrementing the failure > count. The current state is awkward because MinidumpUploadCallable renames > successful crashes, but not failed ones. I added thorough documentation, > though, in hopes that it will help (a) clarify and (b) alert the reader to be > careful. Do you think that's sufficient for now? Yeah, that looks fine ;). https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:55: // suffix... On 2017/03/15 02:13:35, Ilya Sherman wrote: > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > On 2017/03/14 02:22:03, Ilya Sherman wrote: > > > By the way, Gustav, I noticed that uploaded files didn't seem to be being > > > deleted by the cleanup code... and found what I think might be a bug: The > > > pattern for uploaded files doesn't match the actual uploaded filenames. I'm > > > guessing this is probably pretty important to fix for Webview for M58, yeah? > > > > Huh, that would indeed be important to fix for 58 :) (and to get it into some > > part of the Beta). > > Could you please file a bug? > > I can try to take a look tomorrow (but after that I'll be OOO for the rest of > > the week :/). > > One better: I sent you a CL =) https://codereview.chromium.org/2751903002/ Awesome, thanks a lot! :) (you can remove this comment from the latest PS now :P) https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:54: .setBackoffCriteria(JOB_INITIAL_BACKOFF_TIME_IN_MS, JOB_BACKOFF_POLICY) On 2017/03/15 02:13:35, Ilya Sherman wrote: > On 2017/03/14 18:17:29, gsennton_OOO_Mar16-17 wrote: > > Maybe assert that we haven't set a back-off criteria already? > > I think it's a good idea, but I'm not seeing any way in the API to read the > currently set value, so I'm not sure how to do this. Oh, right, I guess you would have to build a separate JobInfo before adding your own backoff-criteria to the builder (and then just throw that JobInfo away). It's not super-important :P https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnableTest.java (right): https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnableTest.java:57: private final JobScheduler mRealScheduler; I don't quite see why you have a real JobScheduler in here - you aren't actually checking that the real jobscheduler is working? (and doing so would probably be bad since we don't know how long the jobscheduler will wait before running a job). https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:594: private final JobScheduler mRealScheduler; Here as well, I don't quite see why we are using a real jobscheduler - we don't actually want to schedule any jobs during tests, right?
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 checked by isherman@chromium.org to run a CQ dry run
isherman@chromium.org changed reviewers: + nyquist@chromium.org
+Tommy for OWNERS approval for the background_task_scheduler changes and DEPS additions. I've gone ahead and updated this CL to only enable the JobScheduler on Android M+ (rather than L+), per Tommy's recommendation. https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java (right): https://codereview.chromium.org/2737263006/diff/40001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java:55: // suffix... On 2017/03/15 12:57:43, gsennton_OOO_Mar16-17 wrote: > On 2017/03/15 02:13:35, Ilya Sherman wrote: > > On 2017/03/14 18:17:28, gsennton_OOO_Mar16-17 wrote: > > > On 2017/03/14 02:22:03, Ilya Sherman wrote: > > > > By the way, Gustav, I noticed that uploaded files didn't seem to be being > > > > deleted by the cleanup code... and found what I think might be a bug: The > > > > pattern for uploaded files doesn't match the actual uploaded filenames. > I'm > > > > guessing this is probably pretty important to fix for Webview for M58, > yeah? > > > > > > Huh, that would indeed be important to fix for 58 :) (and to get it into > some > > > part of the Beta). > > > Could you please file a bug? > > > I can try to take a look tomorrow (but after that I'll be OOO for the rest > of > > > the week :/). > > > > One better: I sent you a CL =) https://codereview.chromium.org/2751903002/ > > Awesome, thanks a lot! :) > (you can remove this comment from the latest PS now :P) Done. https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnableTest.java (right): https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/LogcatExtractionRunnableTest.java:57: private final JobScheduler mRealScheduler; On 2017/03/15 12:57:43, gsennton_OOO_Mar16-17 wrote: > I don't quite see why you have a real JobScheduler in here - you aren't actually > checking that the real jobscheduler is working? (and doing so would probably be > bad since we don't know how long the jobscheduler will wait before running a > job). Hmm, good point. Simplified. https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java (right): https://codereview.chromium.org/2737263006/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:594: private final JobScheduler mRealScheduler; On 2017/03/15 12:57:43, gsennton_OOO_Mar16-17 wrote: > Here as well, I don't quite see why we are using a real jobscheduler - we don't > actually want to schedule any jobs during tests, right? Done.
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:20: @TargetApi(Build.VERSION_CODES.LOLLIPOP) switch to Marshmallow? https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("Couldn't schedule " + uploadJob); hmm, when can this happen? If I am reading it correctly, this is the function that gets called from the deferred handler. So throwing an exception here will crash chrome. That seems like a weird thing to do if we couldn't schedule uploads for crash reports...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:20: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2017/03/16 04:10:43, Maria wrote: > switch to Marshmallow? This class is shared between Chrome and Android Webview, so I left it at Lollipop, which is what Webview is targeting. https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("Couldn't schedule " + uploadJob); On 2017/03/16 04:10:43, Maria wrote: > hmm, when can this happen? If I am reading it correctly, this is the function > that gets called from the deferred handler. So throwing an exception here will > crash chrome. That seems like a weird thing to do if we couldn't schedule > uploads for crash reports... It can only happen if the job is misconfigured, i.e if the builder is given an invalid set of parameters. Would you prefer this to be handled somehow other than by throwing an exception? This should only ever fail deterministically. I agree that a crash loop would be bad, though -- perhaps logging an error makes more sense? (This is code that I moved from the Android Webview implementation, fwiw.)
lgtm https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("Couldn't schedule " + uploadJob); On 2017/03/16 05:21:03, Ilya Sherman wrote: > On 2017/03/16 04:10:43, Maria wrote: > > hmm, when can this happen? If I am reading it correctly, this is the function > > that gets called from the deferred handler. So throwing an exception here will > > crash chrome. That seems like a weird thing to do if we couldn't schedule > > uploads for crash reports... > > It can only happen if the job is misconfigured, i.e if the builder is given an > invalid set of parameters. Would you prefer this to be handled somehow other > than by throwing an exception? This should only ever fail deterministically. I > agree that a crash loop would be bad, though -- perhaps logging an error makes > more sense? (This is code that I moved from the Android Webview implementation, > fwiw.) Hmm, how about an assert instead? It will alert the developers early, but won't get triggered in production builds.
android_webview LGTM.
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { Would it be possible to use BackgroundTask here instead?
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 14:52:18, nyquist wrote: > Would it be possible to use BackgroundTask here instead? This code is shared with the Android Webview implementation, so I'd prefer not to switch over to BackgroundTask for M58 (which this code is destined to be merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: does BackgroundTask support Android L? https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("Couldn't schedule " + uploadJob); On 2017/03/16 05:45:19, Maria wrote: > On 2017/03/16 05:21:03, Ilya Sherman wrote: > > On 2017/03/16 04:10:43, Maria wrote: > > > hmm, when can this happen? If I am reading it correctly, this is the > function > > > that gets called from the deferred handler. So throwing an exception here > will > > > crash chrome. That seems like a weird thing to do if we couldn't schedule > > > uploads for crash reports... > > > > It can only happen if the job is misconfigured, i.e if the builder is given an > > invalid set of parameters. Would you prefer this to be handled somehow other > > than by throwing an exception? This should only ever fail deterministically. > I > > agree that a crash loop would be bad, though -- perhaps logging an error makes > > more sense? (This is code that I moved from the Android Webview > implementation, > > fwiw.) > > Hmm, how about an assert instead? It will alert the developers early, but won't > get triggered in production builds. Okay, I'll check later today whether an assert would have triggered in my build configuration. I'm a little concerned that asserts are going to tend to be disabled in the near-Official-Release build configuration that's needed for the crash reporting code to actually run. If an assert doesn't trigger in that configuration, do you have any preferences for a backup option?
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:57: throw new RuntimeException("Couldn't schedule " + uploadJob); On 2017/03/16 17:46:29, Ilya Sherman wrote: > On 2017/03/16 05:45:19, Maria wrote: > > On 2017/03/16 05:21:03, Ilya Sherman wrote: > > > On 2017/03/16 04:10:43, Maria wrote: > > > > hmm, when can this happen? If I am reading it correctly, this is the > > function > > > > that gets called from the deferred handler. So throwing an exception here > > will > > > > crash chrome. That seems like a weird thing to do if we couldn't schedule > > > > uploads for crash reports... > > > > > > It can only happen if the job is misconfigured, i.e if the builder is given > an > > > invalid set of parameters. Would you prefer this to be handled somehow > other > > > than by throwing an exception? This should only ever fail > deterministically. > > I > > > agree that a crash loop would be bad, though -- perhaps logging an error > makes > > > more sense? (This is code that I moved from the Android Webview > > implementation, > > > fwiw.) > > > > Hmm, how about an assert instead? It will alert the developers early, but > won't > > get triggered in production builds. > > Okay, I'll check later today whether an assert would have triggered in my build > configuration. I'm a little concerned that asserts are going to tend to be > disabled in the near-Official-Release build configuration that's needed for the > crash reporting code to actually run. If an assert doesn't trigger in that > configuration, do you have any preferences for a backup option? Actually, I suppose we use asserts in various other places in the crash reporting code as well, and if my build configuration doesn't trigger assertion failures, I should probably fix up my build configuration somehow. So, I went ahead and switched to using an assert here. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 17:46:29, Ilya Sherman wrote: > On 2017/03/16 14:52:18, nyquist wrote: > > Would it be possible to use BackgroundTask here instead? > > This code is shared with the Android Webview implementation, so I'd prefer not > to switch over to BackgroundTask for M58 (which this code is destined to be > merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: does > BackgroundTask support Android L? BackgroundTask does not support Android L yet. Also; for pre-M it will have to use GCM for now. Does WebView also roll in GMS core?
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 18:50:02, nyquist wrote: > On 2017/03/16 17:46:29, Ilya Sherman wrote: > > On 2017/03/16 14:52:18, nyquist wrote: > > > Would it be possible to use BackgroundTask here instead? > > > > This code is shared with the Android Webview implementation, so I'd prefer not > > to switch over to BackgroundTask for M58 (which this code is destined to be > > merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: does > > BackgroundTask support Android L? > > BackgroundTask does not support Android L yet. Okay. In that case it sounds like it wouldn't make sense to switch this over yet -- agreed? > Also; for pre-M it will have to use GCM for now. Does WebView also roll in GMS > core? I'm not sure whether Webview rolls in GMS core. Gustav (gsennton@) would be the best person to discuss with -- he's OOO until next week though.
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 18:56:08, Ilya Sherman wrote: > On 2017/03/16 18:50:02, nyquist wrote: > > On 2017/03/16 17:46:29, Ilya Sherman wrote: > > > On 2017/03/16 14:52:18, nyquist wrote: > > > > Would it be possible to use BackgroundTask here instead? > > > > > > This code is shared with the Android Webview implementation, so I'd prefer > not > > > to switch over to BackgroundTask for M58 (which this code is destined to be > > > merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: does > > > BackgroundTask support Android L? > > > > BackgroundTask does not support Android L yet. > > Okay. In that case it sounds like it wouldn't make sense to switch this over > yet -- agreed? > > > Also; for pre-M it will have to use GCM for now. Does WebView also roll in GMS > > core? > > I'm not sure whether Webview rolls in GMS core. Gustav (gsennton@) would be the > best person to discuss with -- he's OOO until next week though. Also WebView has different requirements to Chrome here; this code runs in a separate service in the WebView package, and so we need to be able to schedule the work for a later point when network/power conditions are favourable, and still be able to have the service exit. I don't think BackgroundTask would give us that ability. We do link the GMSCore client library, but it's only a recent development.
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 19:01:36, Tobias Sargeant wrote: > On 2017/03/16 18:56:08, Ilya Sherman wrote: > > On 2017/03/16 18:50:02, nyquist wrote: > > > On 2017/03/16 17:46:29, Ilya Sherman wrote: > > > > On 2017/03/16 14:52:18, nyquist wrote: > > > > > Would it be possible to use BackgroundTask here instead? > > > > > > > > This code is shared with the Android Webview implementation, so I'd prefer > > not > > > > to switch over to BackgroundTask for M58 (which this code is destined to > be > > > > merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: does > > > > BackgroundTask support Android L? > > > > > > BackgroundTask does not support Android L yet. > > > > Okay. In that case it sounds like it wouldn't make sense to switch this over > > yet -- agreed? > > > > > Also; for pre-M it will have to use GCM for now. Does WebView also roll in > GMS > > > core? > > > > I'm not sure whether Webview rolls in GMS core. Gustav (gsennton@) would be > the > > best person to discuss with -- he's OOO until next week though. > > Also WebView has different requirements to Chrome here; this code runs in a > separate service in the WebView package, and so we need to be able to schedule > the work for a later point when network/power conditions are favourable, and > still be able to have the service exit. I don't think BackgroundTask would give > us that ability. > > We do link the GMSCore client library, but it's only a recent development. I'm not sure what you mean? For now BackgroundTask is mostly a wrapper around JobService. It has the same ability to run later. It still requires an entry in the manifest though. See: https://chromium.googlesource.com/chromium/src/+/master/components/background...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java (right): https://codereview.chromium.org/2737263006/diff/160001/components/minidump_up... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploadJobService.java:21: public abstract class MinidumpUploadJobService extends JobService { On 2017/03/16 20:06:26, nyquist wrote: > On 2017/03/16 19:01:36, Tobias Sargeant wrote: > > On 2017/03/16 18:56:08, Ilya Sherman wrote: > > > On 2017/03/16 18:50:02, nyquist wrote: > > > > On 2017/03/16 17:46:29, Ilya Sherman wrote: > > > > > On 2017/03/16 14:52:18, nyquist wrote: > > > > > > Would it be possible to use BackgroundTask here instead? > > > > > > > > > > This code is shared with the Android Webview implementation, so I'd > prefer > > > not > > > > > to switch over to BackgroundTask for M58 (which this code is destined to > > be > > > > > merged to). For M59 (i.e. in a follow-up CL), possibly. It depends: > does > > > > > BackgroundTask support Android L? > > > > > > > > BackgroundTask does not support Android L yet. > > > > > > Okay. In that case it sounds like it wouldn't make sense to switch this > over > > > yet -- agreed? > > > > > > > Also; for pre-M it will have to use GCM for now. Does WebView also roll in > > GMS > > > > core? > > > > > > I'm not sure whether Webview rolls in GMS core. Gustav (gsennton@) would be > > the > > > best person to discuss with -- he's OOO until next week though. > > > > Also WebView has different requirements to Chrome here; this code runs in a > > separate service in the WebView package, and so we need to be able to schedule > > the work for a later point when network/power conditions are favourable, and > > still be able to have the service exit. I don't think BackgroundTask would > give > > us that ability. > > > > We do link the GMSCore client library, but it's only a recent development. > > I'm not sure what you mean? For now BackgroundTask is mostly a wrapper around > JobService. It has the same ability to run later. It still requires an entry in > the manifest though. > > See: > https://chromium.googlesource.com/chromium/src/+/master/components/background... Sorry, I spoke too quickly without looking into how BackgroundTask was implemented. Thanks for the docs pointer.
//components/background_task_scheduler and DEPS on it LGTM We can look into using the BackgroundTaskScheduler for all of this later.
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gsennton@chromium.org, mariakhomenko@chromium.org, tobiasjs@chromium.org Link to the patchset: https://codereview.chromium.org/2737263006/#ps180001 (title: "Assert that job scheduled successfully")
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": 1489702407425180,
"parent_rev": "5db93ce184722588b24a21fc2f9c53b16cf9a45b", "commit_rev":
"8a5492dc16ac148bde50b46ca756f95a655fad58"}
Message was sent while issue was closed.
Description was changed from ========== [Android Crash Reporting] Allow uploading minidumps via the JobScheduler The new functionality is guarded behind a Feature. BUG=694884 R=gsennton@chromium.org, mariakhomenko@chromium.org ========== to ========== [Android Crash Reporting] Allow uploading minidumps via the JobScheduler The new functionality is guarded behind a Feature. BUG=694884 R=gsennton@chromium.org, mariakhomenko@chromium.org Review-Url: https://codereview.chromium.org/2737263006 Cr-Commit-Position: refs/heads/master@{#457584} Committed: https://chromium.googlesource.com/chromium/src/+/8a5492dc16ac148bde50b46ca756... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/8a5492dc16ac148bde50b46ca756... |
