[Android WebView] Ensure we use free connection when uploading minidumps

[Android WebView] Ensure we use free connection when uploading minidumps

By using the JobScheduler to upload minidumps we are restricting
ourselves to not start a job uploading minidumps on costly connections
(we allow only unmetered connections). In this CL we explicitly ensure
that we are using an unmetered connection just before performing
uploads as well to ensure we aren't hitting a race where we just
switched connection and the JobScheduler hasn't yet shut our job down.

NOTE: This commit adds the permission
android.permission.ACCESS_NETWORK_STATE to WebView.

BUG=678924
BUG=673280
Review-Url: https://codereview.chromium.org/2666603002
Cr-Commit-Position: refs/heads/master@{#447972}
Committed: https://chromium.googlesource.com/chromium/src/+/49a6b5d0a4738663b0740324b4666a70eec295f2

[gsennton, 2017-01-30 17:38:46]

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

[gsennton, 2017-02-02 14:04:22]

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

[gsennton, 2017-02-02 14:04:46]

On 2017/02/02 14:04:23, gsennton wrote:

Given that Toby is home, Selim ptal :)

[sgurun-gerrit only, 2017-02-02 14:12:03]

https://codereview.chromium.org/2666603002/diff/1/android_webview/apk/java/An...
File android_webview/apk/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2666603002/diff/1/android_webview/apk/java/An...
android_webview/apk/java/AndroidManifest.xml:19: <uses-permission
android:name="android.permission.ACCESS_NETWORK_STATE" />
will users who are getting an update to webview receive a dialog for extra
permission?

please verify

[gsennton, 2017-02-02 14:19:31]

On 2017/02/02 14:12:03, sgurun wrote:
>
https://codereview.chromium.org/2666603002/diff/1/android_webview/apk/java/An...
> File android_webview/apk/java/AndroidManifest.xml (right):
> 
>
https://codereview.chromium.org/2666603002/diff/1/android_webview/apk/java/An...
> android_webview/apk/java/AndroidManifest.xml:19: <uses-permission
> android:name="android.permission.ACCESS_NETWORK_STATE" />
> will users who are getting an update to webview receive a dialog for extra
> permission?
> 
> please verify

Right, we did test that adding android.permission.INTERNET didn't mess with
auto-updates in any way, and the same should be true for ACCESS_NETWORK_STATE,
see chatty discussion here
https://groups.google.com/a/google.com/forum/#!topic/android-chatty-eng/0Zlvz...
for clarification on this. To verify this in reality we have to actually land
this CL first (since adb-installing an apk won't pop up any dialog anyway
AFAIK).

The results from our test team testing auto-updates for
android.permission.INTERNET are here:
https://docs.google.com/a/google.com/spreadsheets/d/1lZjxq-uQeUhttEOoApAPcBSE...

[sgurun-gerrit only, 2017-02-02 16:39:21]

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:101:
|| networkType == ConnectivityManager.TYPE_ETHERNET);
2 questions: when are we going to upload for mobile connections?
isn't it better to use ConnectivityManager.isActiveNetworkMetered()?

[gsennton, 2017-02-02 17:27:10]

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:101:
|| networkType == ConnectivityManager.TYPE_ETHERNET);
On 2017/02/02 16:39:21, sgurun wrote:
> 2 questions: when are we going to upload for mobile connections?
> isn't it better to use ConnectivityManager.isActiveNetworkMetered()?
> 

How do you mean 'when are we going to upload for mobile connections'? We ask the
JobScheduler to schedule jobs when we are on unmetered connections here:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

by calling

JobInfo newJob = new JobInfo
                .Builder(MINIDUMP_UPLOADING_JOB_ID /* jobId */,
                    new ComponentName(this, MinidumpUploadJobService.class))
                .setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED)...

I didn't know we had an isMetered() method :), thanks.

[sgurun-gerrit only, 2017-02-02 17:50:12]

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2666603002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:101:
|| networkType == ConnectivityManager.TYPE_ETHERNET);
On 2017/02/02 17:27:10, gsennton wrote:
> On 2017/02/02 16:39:21, sgurun wrote:
> > 2 questions: when are we going to upload for mobile connections?
> > isn't it better to use ConnectivityManager.isActiveNetworkMetered()?
> > 
> 
> How do you mean 'when are we going to upload for mobile connections'? We ask
the
> JobScheduler to schedule jobs when we are on unmetered connections here:
>
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

That is exactly what I am asking :) . So if somebody is on a metered network,
and never connects to wifi, then we will never receive minidumps right? Let's
sync offline. 

> 
> by calling
> 
> JobInfo newJob = new JobInfo
>                 .Builder(MINIDUMP_UPLOADING_JOB_ID /* jobId */,
>                     new ComponentName(this, MinidumpUploadJobService.class))
>                 .setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED)...
> 
> I didn't know we had an isMetered() method :), thanks.

[sgurun-gerrit only, 2017-02-02 20:44:06]

lgtm

[gsennton, 2017-02-03 09:22:13]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2017-02-03 09:22:20]

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-02-03 09:45:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486113733751390,
"parent_rev": "6fb1a6ff9319dbf7e9dd8d165f675ed398160db2", "commit_rev":
"49a6b5d0a4738663b0740324b4666a70eec295f2"}

[commit-bot: I haz the power, 2017-02-03 09:45:40]

Description was changed from

==========
[Android WebView] Ensure we use free connection when uploading minidumps

By using the JobScheduler to upload minidumps we are restricting
ourselves to not start a job uploading minidumps on costly connections
(we allow only unmetered connections). In this CL we explicitly ensure
that we are using an unmetered connection just before performing
uploads as well to ensure we aren't hitting a race where we just
switched connection and the JobScheduler hasn't yet shut our job down.

NOTE: This commit adds the permission
android.permission.ACCESS_NETWORK_STATE to WebView.

BUG=678924
BUG=673280
==========

to

==========
[Android WebView] Ensure we use free connection when uploading minidumps

By using the JobScheduler to upload minidumps we are restricting
ourselves to not start a job uploading minidumps on costly connections
(we allow only unmetered connections). In this CL we explicitly ensure
that we are using an unmetered connection just before performing
uploads as well to ensure we aren't hitting a race where we just
switched connection and the JobScheduler hasn't yet shut our job down.

NOTE: This commit adds the permission
android.permission.ACCESS_NETWORK_STATE to WebView.

BUG=678924
BUG=673280

Review-Url: https://codereview.chromium.org/2666603002
Cr-Commit-Position: refs/heads/master@{#447972}
Committed:
https://chromium.googlesource.com/chromium/src/+/49a6b5d0a4738663b0740324b466...
==========

[commit-bot: I haz the power, 2017-02-03 09:45:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/49a6b5d0a4738663b0740324b466...