[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

When re-scheduling minidump uploading in MinidumpUploadRetry we
currently re-try uploading whenever we receive a network-change and the
new connection type is a valid connection. This could result in losing
minidumps if the connection is not of WiFi/Ethernet type since we avoid
uploading minidumps over any other type of connection - and since after
failing to upload a minidump 3 times we abandon the minidump.

BUG=673097
Committed: https://crrev.com/1384ca7b54c0a6827e64921a5eccd6c7c84e7391
Cr-Commit-Position: refs/heads/master@{#441213}

[gsennton, 2016-12-22 17:15:35]

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

[gsennton, 2016-12-22 17:15:35]

Hi Ilya, do you wanna have a look at this before I ask chrome/android/ & net/
reviewers to have a look? :)

Also, I couldn't really come up with a better way to not regress this than
adding a comment in PrivacyPreferencesManager to ask developers not to change
one "isNetworkAvailableForCrashUploads" method without also changing the other.
If you have any better suggestions please let me know ;)

[Ilya Sherman, 2016-12-22 21:55:40]

Thanks for looking into this, Gustav!

https://codereview.chromium.org/2599063002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/2599063002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:225:
}
Hmm, could the MinidumpUploadRetry code ignore the |connectionType| passed into
it, and instead just check isNetworkAvailable() && isWiFiOrEthernetNetwork()?

[gsennton, 2016-12-23 14:12:11]

There, I added a CrashReportingManager that we pass from MinidumpUploadService
to MinidumpUploadRetry instead of just calling PrivacyManager directly
(otherwise our instrumentation tests should start depending on the actual
network state of the test-device - which seems wrong :)).

https://codereview.chromium.org/2599063002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java
(right):

https://codereview.chromium.org/2599063002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:225:
}
On 2016/12/22 21:55:40, Ilya Sherman (Away De.29-Ja.4) wrote:
> Hmm, could the MinidumpUploadRetry code ignore the |connectionType| passed
into
> it, and instead just check isNetworkAvailable() && isWiFiOrEthernetNetwork()?

Hah, good point! >.<
Now I feel bad for not thinking of that myself ;)

[Ilya Sherman, 2016-12-24 01:20:49]

Thanks!  LGTM % a quibble or two:

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java
(right):

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java:27:
private final CrashReportingPermissionManager mPermManager;
nit: I'd prefer to spell out "Permission" (throughout this file), but I'm not
sure whether there's a broader style preference on this.  (In C++ code, I
believe the style guide recommendation is to spell out variable names in
general.)

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java
(right):

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:96:
private Object mLock = new Object();
Huh, do the tests really need a lock?  Seems surprising -- are there race
conditions in the tests?

[gsennton, 2017-01-03 18:25:58]

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

[gsennton, 2017-01-03 18:25:59]

mariakhomenko@chromium.org ptal at EVERYTHING ;)

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java
(right):

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java:27:
private final CrashReportingPermissionManager mPermManager;
On 2016/12/24 01:20:49, Ilya Sherman (Away De.29-Ja.4) wrote:
> nit: I'd prefer to spell out "Permission" (throughout this file), but I'm not
> sure whether there's a broader style preference on this.  (In C++ code, I
> believe the style guide recommendation is to spell out variable names in
> general.)

Done.

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java
(right):

https://codereview.chromium.org/2599063002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:96:
private Object mLock = new Object();
On 2016/12/24 01:20:49, Ilya Sherman (Away De.29-Ja.4) wrote:
> Huh, do the tests really need a lock?  Seems surprising -- are there race
> conditions in the tests?

I added a lock because I wasn't sure whether the setter and the getter would be
called on different threads. It looks like they are both called on the UI thread
atm, so we don't really need the lock :)

Removing the locking mechanism.

[Maria, 2017-01-03 19:28:41]

lgtm

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java
(right):

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java:69:
if (!mPermissionManager.isNetworkAvailableForCrashUploads()) {
As a small optimization I would keep the check for connectionType==NONE here. We
will definitely not be able to upload anything without a connection and this way
we'll avoid the extra system calls in the permission manager.

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java
(right):

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:83:
private NetworkChangingPermissionManager mPermissionManager =
final

[gsennton, 2017-01-03 20:00:35]

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java
(right):

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadRetry.java:69:
if (!mPermissionManager.isNetworkAvailableForCrashUploads()) {
On 2017/01/03 19:28:41, Maria wrote:
> As a small optimization I would keep the check for connectionType==NONE here.
We
> will definitely not be able to upload anything without a connection and this
way
> we'll avoid the extra system calls in the permission manager.

Done.

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java
(right):

https://codereview.chromium.org/2599063002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java:83:
private NetworkChangingPermissionManager mPermissionManager =
On 2017/01/03 19:28:41, Maria wrote:
> final

Done.

[gsennton, 2017-01-03 20:12:36]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-01-03 20:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2599063002/#ps60001
(title: "Fix Maria's comments (early out from retries if there is no
connection).")

[commit-bot: I haz the power, 2017-01-03 20:13:02]

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

[commit-bot: I haz the power, 2017-01-03 21:43:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483474356719250,
"parent_rev": "a85c6636acc96b4f97f52551c3c22a65197f0528", "commit_rev":
"d3e4f061e069760c51928bdaddb192d830a51d10"}

[commit-bot: I haz the power, 2017-01-03 21:43:49]

Description was changed from

==========
[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

When re-scheduling minidump uploading in MinidumpUploadRetry we
currently re-try uploading whenever we receive a network-change and the
new connection type is a valid connection. This could result in losing
minidumps if the connection is not of WiFi/Ethernet type since we avoid
uploading minidumps over any other type of connection - and since after
failing to upload a minidump 3 times we abandon the minidump.

BUG=673097
==========

to

==========
[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

When re-scheduling minidump uploading in MinidumpUploadRetry we
currently re-try uploading whenever we receive a network-change and the
new connection type is a valid connection. This could result in losing
minidumps if the connection is not of WiFi/Ethernet type since we avoid
uploading minidumps over any other type of connection - and since after
failing to upload a minidump 3 times we abandon the minidump.

BUG=673097

Review-Url: https://codereview.chromium.org/2599063002
==========

[commit-bot: I haz the power, 2017-01-03 21:43:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-03 21:46:20]

Description was changed from

==========
[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

When re-scheduling minidump uploading in MinidumpUploadRetry we
currently re-try uploading whenever we receive a network-change and the
new connection type is a valid connection. This could result in losing
minidumps if the connection is not of WiFi/Ethernet type since we avoid
uploading minidumps over any other type of connection - and since after
failing to upload a minidump 3 times we abandon the minidump.

BUG=673097

Review-Url: https://codereview.chromium.org/2599063002
==========

to

==========
[minidump uploader] Retries: only initiate uploads when on WiFi/Ethernet

When re-scheduling minidump uploading in MinidumpUploadRetry we
currently re-try uploading whenever we receive a network-change and the
new connection type is a valid connection. This could result in losing
minidumps if the connection is not of WiFi/Ethernet type since we avoid
uploading minidumps over any other type of connection - and since after
failing to upload a minidump 3 times we abandon the minidump.

BUG=673097

Committed: https://crrev.com/1384ca7b54c0a6827e64921a5eccd6c7c84e7391
Cr-Commit-Position: refs/heads/master@{#441213}
==========

[commit-bot: I haz the power, 2017-01-03 21:46:22]

Patchset 4 (id:??) landed as
https://crrev.com/1384ca7b54c0a6827e64921a5eccd6c7c84e7391
Cr-Commit-Position: refs/heads/master@{#441213}