Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628863004:
[Android WebView] Ensure we have user consent before uploading minidumps (Closed)
Description[Android WebView] Ensure we have user consent before uploading minidumps
WebView won't upload minidumps unless the Android Checkbox is toggled
on (or the enabled-for-testing command-line flag is toggled on and we
are using a debuggable device).
With this CL we check the Checkbox value (in the app process) just
before copying minidumps to the separate minidump uploading service. If
we do not have user consent the minidumps are deleted.
BUG=657314
BUG=679693
BUG=683882
Review-Url: https://codereview.chromium.org/2628863004
Cr-Commit-Position: refs/heads/master@{#446432}
Committed: https://chromium.googlesource.com/chromium/src/+/672f4a2e91db3968619c5b46d6c466270584fdb6
Patch Set 1 #
Total comments: 11
Patch Set 2 : Check user consent before copying minidumps, delete minidumps in app dir if no consent, add unit te… #
Total comments: 15
Patch Set 3 : Move consent-value checking into its own private class. #
Total comments: 5
Patch Set 4 : Remove SyncWVCommandLine (instead only use CommandLine on main thread), remove unnecessary unit tes… #Patch Set 5 : Minor cleanups + comment fixes. #
Total comments: 14
Patch Set 6 : Remove command-line + gms core usage from minidump-uploading services. #Patch Set 7 : Commit message change. #Messages
Total messages: 29 (8 generated)
gsennton@chromium.org changed reviewers: + paulmiller@chromium.org, sgurun@chromium.org, tobiasjs@chromium.org
This CL depends on Paul's change in https://codereview.chromium.org/2611883002/ (patch set 4). It would be great to get this landed before the 57 branch point, but since I'm OOO next week I'm uploading this now to let you guys have a look :)
https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: if (platformServiceBridge.canUseGms()) { // TODO delete minidumps if canUseGms == false? Or maybe disable generating them in the first place...
https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:416: AwBrowserProcess.handleMinidumps(webViewPackageName, enabled); Metrics also needs to query this on startup. I'm doing it at the end of the AwContents constructor in my latest patch set: https://codereview.chromium.org/2611883002/patch/80001/90007 although that's probably not the best place for it. Anyway, we should eventually combine the two calls.
https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java:12: public interface UserConsentInterface { Is this class really necessary?
https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:176: if (!userApproved) { TODO also read enableForTesting flag (to have it override userApproved). https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java:12: public interface UserConsentInterface { On 2017/01/13 08:14:21, sgurun wrote: > Is this class really necessary? Its only use is to allow me to mock PlatformServicesBridge. So, probably no :) https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:196: if (enabled) { TODO also read enabledForTesting flag and have it override userPermission
On 2017/01/13 19:01:55, gsennton wrote: > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java > (right): > > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:176: > if (!userApproved) { > TODO also read enableForTesting flag (to have it override userApproved). > > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > File > android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java > (right): > > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java:12: > public interface UserConsentInterface { > On 2017/01/13 08:14:21, sgurun wrote: > > Is this class really necessary? > > Its only use is to allow me to mock PlatformServicesBridge. So, probably no :) > > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > File > android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java > (right): > > https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:196: > if (enabled) { > TODO also read enabledForTesting flag and have it override userPermission is this ready for review?
Description was changed from ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on. With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) just before uploading a batch of minidumps we ensure that we have user consent. BUG=657314 ========== to ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on. With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. BUG=657314 BUG=679693 ==========
Description was changed from ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on. With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. BUG=657314 BUG=679693 ========== to ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. This CL also ensures that we do not read command-line flags in the separate WebView process unless we are using a debuggable device. BUG=657314 BUG=679693 ==========
ptal :) https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: if (platformServiceBridge.canUseGms()) { // TODO delete minidumps if canUseGms == false? On 2017/01/12 13:56:54, Tobias Sargeant wrote: > Or maybe disable generating them in the first place... Offline we reached the conclusion to just delete minidumps here if canUseGms() is false. When minidump sanitization is done we will only enable minidump generation for official builds. https://codereview.chromium.org/2628863004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:416: AwBrowserProcess.handleMinidumps(webViewPackageName, enabled); On 2017/01/12 22:32:48, paulmiller wrote: > Metrics also needs to query this on startup. I'm doing it at the end of the > AwContents constructor in my latest patch set: > https://codereview.chromium.org/2611883002/patch/80001/90007 > although that's probably not the best place for it. Anyway, we should eventually > combine the two calls. Let's do that for 58, rather than in this CL. Added crbug.com/683882 https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:176: if (!userApproved) { On 2017/01/13 19:01:54, gsennton wrote: > TODO also read enableForTesting flag (to have it override userApproved). Done (in WebViewFactoryProvider). https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/UserConsentInterface.java:12: public interface UserConsentInterface { On 2017/01/13 19:01:55, gsennton wrote: > On 2017/01/13 08:14:21, sgurun wrote: > > Is this class really necessary? > > Its only use is to allow me to mock PlatformServicesBridge. So, probably no :) Removed this now. https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2628863004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:196: if (enabled) { On 2017/01/13 19:01:55, gsennton wrote: > TODO also read enabledForTesting flag and have it override userPermission Done (in CrashReceiverService).
https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: // Copy minidumps if Android Checkbox is toggled on. TODO merge this with the queryMetricsSetting call below.
https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:407: CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH); Is this only for convenience, to ease the manual testing so you don't have to go to Google settings to set the switch? I am a little bit skeptical of adding a switch for that purpose. Assuming the switch is useful, which I am not convinced yet, I think it might be better to have a switch to overwrite user consent with a false/true. Depending on the goal you may move this logic to PlatformServiceBridgeGoogle (if switch exists return the value of switch, rather than GMS) this can also allow merging the logic below in a simple way. https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: // Copy minidumps if Android Checkbox is toggled on. On 2017/01/23 18:33:53, gsennton wrote: > TODO merge this with the queryMetricsSetting call below. +1 https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:55: private int mConsentValueState = CONSENT_NOT_RETURNED; nit: rename to mConsentValue https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:74: private boolean getConsentValue() { would it be better to keep the methods that provide access to mConsentValue in their own encapsulation, i.e in a nested class where only get/set are public? https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/SynchronizedWebViewCommandLine.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/SynchronizedWebViewCommandLine.java:23: return new SynchronizedWebViewCommandLine(); why is it necessary to be able to create different instances of SynchronizedWebViewCommandLine?
https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:407: CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH); On 2017/01/24 08:58:49, sgurun wrote: > Is this only for convenience, to ease the manual testing so you don't have to go > to Google settings to set the switch? I am a little bit skeptical of adding a > switch for that purpose. > > Assuming the switch is useful, which I am not convinced yet, I think it might be > better to have a switch to overwrite user consent with a false/true. Depending > on the goal you may move this logic to PlatformServiceBridgeGoogle (if switch > exists return the value of switch, rather than GMS) > > this can also allow merging the logic below in a simple way. It's also useful for devices that do not have the newest gms-core apk - where it doesn't matter whether we turn the checkbox on or off since we can't fetch its value from WebView either way. Right now the switch is also used within MinidumpUploaderImpl, though that seems unnecessary given that the switch is checked in CrashReceiverService (i.e. before copying minidumps). Given that we could probably settle with reading the command-line flag from within CrashReceiverService (and WebViewFactoryProvider), and given that the flag will only be read on debuggable builds - maybe we should do just as in WebViewFactoryProvider, i.e. initialize and use the command only on the main thread (since we will only read from a command-line-file in debuggable builds) and enable strictmode if initializing the command-line from a file. (this would mean removing the entire SynchronizedWebViewCommandLine class) WDYT? Maybe, as you say, we want a flag for turning on user consent (instead of depending on the current flag). We are currently depending on the flag 'enable-crash-reporter-for-testing' which is also used for enabling breakpad on debug builds. https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: // Copy minidumps if Android Checkbox is toggled on. On 2017/01/24 08:58:49, sgurun wrote: > On 2017/01/23 18:33:53, gsennton wrote: > > TODO merge this with the queryMetricsSetting call below. > > +1 > Why aren't we checking canUseGms() in the call below? https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:55: private int mConsentValueState = CONSENT_NOT_RETURNED; On 2017/01/24 08:58:49, sgurun wrote: > nit: rename to mConsentValue Done. https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:74: private boolean getConsentValue() { On 2017/01/24 08:58:49, sgurun wrote: > would it be better to keep the methods that provide access to mConsentValue in > their own encapsulation, i.e in a nested class where only get/set are public? Done. https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/SynchronizedWebViewCommandLine.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/SynchronizedWebViewCommandLine.java:23: return new SynchronizedWebViewCommandLine(); On 2017/01/24 08:58:49, sgurun wrote: > why is it necessary to be able to create different instances of > SynchronizedWebViewCommandLine? To be able to mock the behaviour of SynchronizedWebViewCommandLine (see CrashReceiverServiceUnitTest.java). It's not strictly necessary to create different instances - all I want is the initOnSeparateThread, and hasSwitch methods to be mockable, and before this change they weren't since they were static.
https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:414: if (platformServiceBridge.canUseGms()) { canUseGms() is unnecessary. If it's false (meaning no closed-source implementation was found), then queryMetricsSetting() will just give you false. The idea was that upstream code could use canUseGms() before directly calling GMS. But if upstream uses GMS indirectly through queryMetricsSetting, then it shouldn't have to care whether that's internally using GMS or not. (It's confusing. Sorry.) https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: AwBrowserProcess.handleMinidumps(webViewPackageName, false /* enabled */); So what happens to the minidump files if we call queryMetricsSetting, but the result never comes back (because GMS dies, or WebView stops before the result arrives)? Will they eventually get deleted?
https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:407: CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH); On 2017/01/24 10:41:47, gsennton wrote: > On 2017/01/24 08:58:49, sgurun wrote: > > Is this only for convenience, to ease the manual testing so you don't have to > go > > to Google settings to set the switch? I am a little bit skeptical of adding a > > switch for that purpose. > > > > Assuming the switch is useful, which I am not convinced yet, I think it might > be > > better to have a switch to overwrite user consent with a false/true. > Depending > > on the goal you may move this logic to PlatformServiceBridgeGoogle (if switch > > exists return the value of switch, rather than GMS) > > > > this can also allow merging the logic below in a simple way. > > It's also useful for devices that do not have the newest gms-core apk - where it > doesn't matter whether we turn the checkbox on or off since we can't fetch its > value from WebView either way. > > Right now the switch is also used within MinidumpUploaderImpl, though that seems > unnecessary given that the switch is checked in CrashReceiverService (i.e. > before copying minidumps). > > > Given that we could probably settle with reading the command-line flag from > within CrashReceiverService (and WebViewFactoryProvider), and given that the > flag will only be read on debuggable builds - maybe we should do just as in > WebViewFactoryProvider, i.e. initialize and use the command only on the main > thread (since we will only read from a command-line-file in debuggable builds) > and enable strictmode if initializing the command-line from a file. (this would > mean removing the entire SynchronizedWebViewCommandLine class) WDYT? > > > Maybe, as you say, we want a flag for turning on user consent (instead of > depending on the current flag). We are currently depending on the flag > 'enable-crash-reporter-for-testing' which is also used for enabling breakpad on > debug builds. I think we should be a little bit stingy while adding flags. I am ok with the idea of having the flag for now, but once GMSv10 is released and goes to stable 100% we should remove the flag. Let's add a simple flag in WebViewFactoryProvider, drop synchronizedwebviewcommandline, and get rid of the whole thing when time comes, as I mentioned above. Every once in a while there is a big effort to cleanup flags, see crbug.com/343941. https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: // Copy minidumps if Android Checkbox is toggled on. On 2017/01/24 10:41:47, gsennton wrote: > On 2017/01/24 08:58:49, sgurun wrote: > > On 2017/01/23 18:33:53, gsennton wrote: > > > TODO merge this with the queryMetricsSetting call below. > > > > +1 > > > > Why aren't we checking canUseGms() in the call below? you need to check for canUseGms() only if you don't know whether your WebView is Google WebView or not. For usermetrics and safebrowsing, there are internal code pieces, and they know they are google webview.
https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:414: if (platformServiceBridge.canUseGms()) { On 2017/01/24 17:33:02, paulmiller wrote: > canUseGms() is unnecessary. If it's false (meaning no closed-source > implementation was found), then queryMetricsSetting() will just give you false. > > The idea was that upstream code could use canUseGms() before directly calling > GMS. But if upstream uses GMS indirectly through queryMetricsSetting, then it > shouldn't have to care whether that's internally using GMS or not. (It's > confusing. Sorry.) Ah, alright, gotcha. https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: AwBrowserProcess.handleMinidumps(webViewPackageName, false /* enabled */); On 2017/01/24 17:33:02, paulmiller wrote: > So what happens to the minidump files if we call queryMetricsSetting, but the > result never comes back (because GMS dies, or WebView stops before the result > arrives)? Will they eventually get deleted? If we never hear back from GMS we won't delete the minidumps during the current WebView lifetime - we will however re-run this code during the next WebView-startup (thus ending deleting minidumps, copying minidumps, or, waiting out GMS again).
Description was changed from ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. This CL also ensures that we do not read command-line flags in the separate WebView process unless we are using a debuggable device. BUG=657314 BUG=679693 ========== to ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. This CL also ensures that we do not read command-line flags in the separate WebView process unless we are using a debuggable device. BUG=657314 BUG=679693 BUG=683882 ==========
Alright, this should be fine now. Note that I haven't added a flag specifically for turning on user-consent - I'm still using the enable-crash-reporting-for-testing flag. https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:407: CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH); On 2017/01/24 18:55:27, sgurun wrote: > On 2017/01/24 10:41:47, gsennton wrote: > > On 2017/01/24 08:58:49, sgurun wrote: > > > Is this only for convenience, to ease the manual testing so you don't have > to > > go > > > to Google settings to set the switch? I am a little bit skeptical of adding > a > > > switch for that purpose. > > > > > > Assuming the switch is useful, which I am not convinced yet, I think it > might > > be > > > better to have a switch to overwrite user consent with a false/true. > > Depending > > > on the goal you may move this logic to PlatformServiceBridgeGoogle (if > switch > > > exists return the value of switch, rather than GMS) > > > > > > this can also allow merging the logic below in a simple way. > > > > It's also useful for devices that do not have the newest gms-core apk - where > it > > doesn't matter whether we turn the checkbox on or off since we can't fetch its > > value from WebView either way. > > > > Right now the switch is also used within MinidumpUploaderImpl, though that > seems > > unnecessary given that the switch is checked in CrashReceiverService (i.e. > > before copying minidumps). > > > > > > Given that we could probably settle with reading the command-line flag from > > within CrashReceiverService (and WebViewFactoryProvider), and given that the > > flag will only be read on debuggable builds - maybe we should do just as in > > WebViewFactoryProvider, i.e. initialize and use the command only on the main > > thread (since we will only read from a command-line-file in debuggable builds) > > and enable strictmode if initializing the command-line from a file. (this > would > > mean removing the entire SynchronizedWebViewCommandLine class) WDYT? > > > > > > Maybe, as you say, we want a flag for turning on user consent (instead of > > depending on the current flag). We are currently depending on the flag > > 'enable-crash-reporter-for-testing' which is also used for enabling breakpad > on > > debug builds. > > I think we should be a little bit stingy while adding flags. I am ok with the > idea of having the flag for now, but once GMSv10 is released and goes to stable > 100% we should remove the flag. Let's add a simple flag in > WebViewFactoryProvider, drop synchronizedwebviewcommandline, and get rid of the > whole thing when time comes, as I mentioned above. > > Every once in a while there is a big effort to cleanup flags, see > crbug.com/343941. Which flag are we talking about here now? Are you still for using a separate flag for overwriting user consent? The enable-crash-reporter-for-testing flag will remain since it's used to generate minidumps on debug builds (in Chrome). If we add a flag in WebViewFactoryProvider it should be read in CrashReceiverService as well (with the current PS I'm just reading the enable-crash-reporting-for-testing flag, I'm still not sure whether we would want to add a override-user-consent-for-testing flag instead?). https://codereview.chromium.org/2628863004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:411: // Copy minidumps if Android Checkbox is toggled on. On 2017/01/24 18:55:27, sgurun wrote: > On 2017/01/24 10:41:47, gsennton wrote: > > On 2017/01/24 08:58:49, sgurun wrote: > > > On 2017/01/23 18:33:53, gsennton wrote: > > > > TODO merge this with the queryMetricsSetting call below. > > > > > > +1 > > > > > > > Why aren't we checking canUseGms() in the call below? > > you need to check for canUseGms() only if you don't know whether your WebView is > Google WebView or not. For usermetrics and safebrowsing, there are internal code > pieces, and they know they are google webview. Done. https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: AwBrowserProcess.handleMinidumps(webViewPackageName, false /* enabled */); On 2017/01/24 19:34:02, gsennton wrote: > On 2017/01/24 17:33:02, paulmiller wrote: > > So what happens to the minidump files if we call queryMetricsSetting, but the > > result never comes back (because GMS dies, or WebView stops before the result > > arrives)? Will they eventually get deleted? > > If we never hear back from GMS we won't delete the minidumps during the current > WebView lifetime - we will however re-run this code during the next > WebView-startup (thus ending deleting minidumps, copying minidumps, or, waiting > out GMS again). Can we end up in a state where we permanently never get a result back from queryMetricsSetting? (i.e. however many times we restart WebView we never get a result).
thanks Gustav, a few more comments. The switch is already in base as you mentioned, I missed it and thought we were introducing for WebView. so it is fine. I also read Paul's comments abouts GMS. I think it should be handled within PlatformServiceBridgeGoogle as multiple features need to read this value. https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:396: final boolean enableMinidumpUploadingForTesting = CommandLine.getInstance().hasSwitch( you can inline this to if https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:403: PlatformServiceBridge.getInstance(context) move to else as no need to do this if command line flag already exists https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:409: if (!enableMinidumpUploadingForTesting) { after moving to else, remove that. https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:84: mLock.wait(); I think we may have a max waiting time here. Do not trust gmscore or any process to come back to you. Alternatively, we can change the platformservicebridgegoogle such that it retries after some time, and if retry does not work within a certain time, returns consent not ok. I am tending towards the latter. I will file a bug to paul for it, so no change needed here. https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:98: mIsCrashCopyingEnabledForTesting = CommandLine.getInstance().hasSwitch( if command line flag has switch, call setConsentValue(true) then you can remove mIsCrashXX variable https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:101: mConsentManager.resetConsentState(); I don't think you need that. You already set the variable to CONSENT_NOT_RETURNED. you can also remove the method above. https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:102: PlatformServiceBridge.getInstance(this).queryMetricsSetting(new ValueCallback<Boolean>() { do this in the else, if command line does not have switch, https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:110: private boolean isCrashCopyingEnabled() { you can then remove this method https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:120: if (!isCrashCopyingEnabled()) { and simply call mContentManager.getConsentValue() here
https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:403: PlatformServiceBridge.getInstance(context) On 2017/01/25 19:42:06, sgurun wrote: > move to else as no need to do this if command line flag already exists This is still needed for AwMetricsServiceClient.setConsentSetting().
https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:102: PlatformServiceBridge.getInstance(this).queryMetricsSetting(new ValueCallback<Boolean>() { On 2017/01/25 19:42:06, sgurun wrote: > do this in the else, if command line does not have switch, Was talking about this with Selim - why do we need another queryMetricsSetting() in the service? Are we sending any dumps to the service before consent is determined? If not, then doesn't receiving dumps indicate to the service that consent was given?
Description was changed from ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value at two points in time 1. (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. 2. (in the separate WebView process) when being asked to copy a set of minidumps we ensure that we have user consent. This CL also ensures that we do not read command-line flags in the separate WebView process unless we are using a debuggable device. BUG=657314 BUG=679693 BUG=683882 ========== to ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. BUG=657314 BUG=679693 BUG=683882 ==========
ptal :) https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:403: PlatformServiceBridge.getInstance(context) On 2017/01/25 21:46:31, paulmiller wrote: > On 2017/01/25 19:42:06, sgurun wrote: > > move to else as no need to do this if command line flag already exists > > This is still needed for AwMetricsServiceClient.setConsentSetting(). I'm doing all of this because we wanted to merge the AwMetricsServiceClient.setConsentSetting() call with the handleMinidumps call into one queryMetricsSetting call (to not call GMS twice). We can't move all of this into an else since AwMetricsServiceClient.setConsentSetting() shouldn't be conditional on enableMinidumpUploadingForTesting. Or am I missing something? This comment holds for all three of your (Selim's) comments in this file :) https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:101: mConsentManager.resetConsentState(); On 2017/01/25 19:42:06, sgurun wrote: > I don't think you need that. You already set the variable to > CONSENT_NOT_RETURNED. > you can also remove the method above. Right, I was under the assumption that onCreate could be called several times for the same Service instance, but that doesn't seem to be the case ;) I'll remove this code anyway given that we don't really need to check the U&D toggle here. https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:102: PlatformServiceBridge.getInstance(this).queryMetricsSetting(new ValueCallback<Boolean>() { On 2017/01/25 23:24:05, paulmiller wrote: > On 2017/01/25 19:42:06, sgurun wrote: > > do this in the else, if command line does not have switch, > > Was talking about this with Selim - why do we need another queryMetricsSetting() > in the service? Are we sending any dumps to the service before consent is > determined? If not, then doesn't receiving dumps indicate to the service that > consent was given? We don't strictly need it - and no we aren't sending any dumps unless the U&D flag (or enableForTesting) are ON, so when only WebView code uses these services we don't need to check the U&D flag within these services. The only possible scenario I'm wondering about is if some app decides to use the minidump-copying API (CrashReceiverService.transmitCrashes) to trigger data uploading to Google servers. Then we would be uploading data because of an app requesting it, without us checking the U&D checkbox. Anyway, I'll remove the U&D + enableForTesting check in this CL for now, and if we feel that we need it anyway I can add it later (so most of the comments here are obsolete now).
On 2017/01/26 10:24:50, gsennton wrote: > ptal :) > > https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > (right): > > https://codereview.chromium.org/2628863004/diff/80001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:403: > PlatformServiceBridge.getInstance(context) > On 2017/01/25 21:46:31, paulmiller wrote: > > On 2017/01/25 19:42:06, sgurun wrote: > > > move to else as no need to do this if command line flag already exists > > > > This is still needed for AwMetricsServiceClient.setConsentSetting(). > > I'm doing all of this because we wanted to merge the > AwMetricsServiceClient.setConsentSetting() call with the handleMinidumps call > into one queryMetricsSetting call (to not call GMS twice). We can't move all of > this into an else since AwMetricsServiceClient.setConsentSetting() shouldn't be > conditional on enableMinidumpUploadingForTesting. Or am I missing something? > my mistake. you are right. > This comment holds for all three of your (Selim's) comments in this file :) > > https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java > (right): > > https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:101: > mConsentManager.resetConsentState(); > On 2017/01/25 19:42:06, sgurun wrote: > > I don't think you need that. You already set the variable to > > CONSENT_NOT_RETURNED. > > you can also remove the method above. > > Right, I was under the assumption that onCreate could be called several times > for the same Service instance, but that doesn't seem to be the case ;) > I'll remove this code anyway given that we don't really need to check the U&D > toggle here. > > https://codereview.chromium.org/2628863004/diff/80001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:102: > PlatformServiceBridge.getInstance(this).queryMetricsSetting(new > ValueCallback<Boolean>() { > On 2017/01/25 23:24:05, paulmiller wrote: > > On 2017/01/25 19:42:06, sgurun wrote: > > > do this in the else, if command line does not have switch, > > > > Was talking about this with Selim - why do we need another > queryMetricsSetting() > > in the service? Are we sending any dumps to the service before consent is > > determined? If not, then doesn't receiving dumps indicate to the service that > > consent was given? > > We don't strictly need it - and no we aren't sending any dumps unless the U&D > flag (or enableForTesting) are ON, so when only WebView code uses these services > we don't need to check the U&D flag within these services. > > The only possible scenario I'm wondering about is if some app decides to use the > minidump-copying API (CrashReceiverService.transmitCrashes) to trigger data > uploading to Google servers. Then we would be uploading data because of an app > requesting it, without us checking the U&D checkbox. I really don't think it is something to worry about. > > Anyway, I'll remove the U&D + enableForTesting check in this CL for now, and if > we feel that we need it anyway I can add it later (so most of the comments here > are obsolete now). It is a lot smaller, less complex and cleaner now. LGTM.
The CQ bit was checked by gsennton@chromium.org
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": 120001, "attempt_start_ts": 1485458420834750,
"parent_rev": "fe0c3d182c49b72798bcf49daec4a425fcfb799f", "commit_rev":
"672f4a2e91db3968619c5b46d6c466270584fdb6"}
Message was sent while issue was closed.
Description was changed from ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. BUG=657314 BUG=679693 BUG=683882 ========== to ========== [Android WebView] Ensure we have user consent before uploading minidumps WebView won't upload minidumps unless the Android Checkbox is toggled on (or the enabled-for-testing command-line flag is toggled on and we are using a debuggable device). With this CL we check the Checkbox value (in the app process) just before copying minidumps to the separate minidump uploading service. If we do not have user consent the minidumps are deleted. BUG=657314 BUG=679693 BUG=683882 Review-Url: https://codereview.chromium.org/2628863004 Cr-Commit-Position: refs/heads/master@{#446432} Committed: https://chromium.googlesource.com/chromium/src/+/672f4a2e91db3968619c5b46d6c4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/672f4a2e91db3968619c5b46d6c4... |
