[Android WebView] Ensure Android Checkbox is on when uploading minidumps

[Android WebView] Ensure Android Checkbox is on when uploading minidumps

Since minidump uploading can happen long after we copy minidumps to
WebView's separate minidump uploading service (and thus long after our
initial check to ensure the Android Checkbox is toggled on), the user
could have toggled the Checkbox off by the time we upload minidumps. We
now ensure the Checkbox is toggled on just before we upload the
minidumps.

This check can be overridden on debuggable builds through the
enable-crash-reporter-for-testing command line flag.

BUG=685576
Review-Url: https://codereview.chromium.org/2655023008
Cr-Commit-Position: refs/heads/master@{#448600}
Committed: https://chromium.googlesource.com/chromium/src/+/30d6e25b2f1c69090883e97110877815f1ae3c72

[gsennton, 2017-01-27 15:57:35]

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

[gsennton, 2017-01-27 15:57:36]

This CL makes it a bit more tricky to componentize some of these classes I think
- but I guess I'll just handle this later :P

[paulmiller, 2017-01-27 18:26:24]

On 2017/01/27 15:57:36, gsennton wrote:
> This CL makes it a bit more tricky to componentize some of these classes I
think
> - but I guess I'll just handle this later :P

Do you want this reviewed yet? It doesn't look like the discussion on the bug
has finished.

[gsennton, 2017-01-30 10:27:20]

On 2017/01/27 18:26:24, paulmiller wrote:
> On 2017/01/27 15:57:36, gsennton wrote:
> > This CL makes it a bit more tricky to componentize some of these classes I
> think
> > - but I guess I'll just handle this later :P
> 
> Do you want this reviewed yet? It doesn't look like the discussion on the bug
> has finished.

Given
crbug.com/685576#c7
and
crbug.com/685576#c8

I believe this CL is needed, but if you are of another opinion please do tell :)

[gsennton, 2017-01-30 10:32:02]

On 2017/01/30 10:27:20, gsennton wrote:
> On 2017/01/27 18:26:24, paulmiller wrote:
> > On 2017/01/27 15:57:36, gsennton wrote:
> > > This CL makes it a bit more tricky to componentize some of these classes I
> > think
> > > - but I guess I'll just handle this later :P
> > 
> > Do you want this reviewed yet? It doesn't look like the discussion on the
bug
> > has finished.
> 
> Given
> crbug.com/685576#c7
> and
> crbug.com/685576#c8
> 
> I believe this CL is needed, but if you are of another opinion please do tell
:)

Actually, leave this be for the moment - given that we won't merge this to 57
(so there's no rush to land it) and given that this CL will clash a little with
how we will componentize minidump uploading so that Chrome can use this
mechanism, we might want to implement this slightly differently (sorry for being
a pain here ;) I was rushing this a bit for 57).

[gsennton, 2017-02-01 17:40:48]

Hey guys, ptal :)

Regarding this clashing with the Chrome-componentization - I've added a way to
componentize this within the componentization doc
(https://docs.google.com/a/google.com/document/d/1PGCEtp_1xXUpfcRdKMajiME-WUDT...).

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:183:
PlatformServiceBridge.getInstance(mContext).queryMetricsSetting(
I'd like some feedback here - do you guys think we should bypass the whole
GmsCore query if the enabledForTesting command-line flag is ON? The reason we
would do so is to avoid the potential issue of GmsCore never calling back to us
(but only in the case where enabled-for-testing is turned on).

[paulmiller, 2017-02-01 18:58:36]

On 2017/02/01 17:40:48, gsennton wrote:
> Hey guys, ptal :)
> 
> Regarding this clashing with the Chrome-componentization - I've added a way to
> componentize this within the componentization doc
>
(https://docs.google.com/a/google.com/document/d/1PGCEtp_1xXUpfcRdKMajiME-WUDT...).
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
> File
>
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
> (right):
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:183:
> PlatformServiceBridge.getInstance(mContext).queryMetricsSetting(
> I'd like some feedback here - do you guys think we should bypass the whole
> GmsCore query if the enabledForTesting command-line flag is ON? The reason we
> would do so is to avoid the potential issue of GmsCore never calling back to
us
> (but only in the case where enabled-for-testing is turned on).

I reviewed the parts I understand but I don't understand which functions are
running on which thread/process. (Maybe some thread assertions would be useful.)

[paulmiller, 2017-02-01 19:26:48]

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java
(right):

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
Does this file need its own directory?

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:36:
if (CommandLineUtil.isBuildDebuggable()) {
Isn't "CommandLineUtil." unnecessary?

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:39:
private volatile boolean mPermittedByUser = false;
mCancelUpload is protected with a lock, but mPermittedByUser is volatile with no
lock. Is there a reason for the difference?

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:183:
PlatformServiceBridge.getInstance(mContext).queryMetricsSetting(
On 2017/02/01 17:40:48, gsennton wrote:
> I'd like some feedback here - do you guys think we should bypass the whole
> GmsCore query if the enabledForTesting command-line flag is ON? The reason we
> would do so is to avoid the potential issue of GmsCore never calling back to
us
> (but only in the case where enabled-for-testing is turned on). 

That would make sense. Not important though, given 1) GmsCore really should call
back and 2) enabled-for-testing isn't a configuration for use out in the wild.

[gsennton, 2017-02-02 14:29:42]

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

[gsennton, 2017-02-02 14:29:43]

Alright, ptal again :)
Adding Torne just for the use of volatile, to ensure I'm not shooting myself in
the foot ;)

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java
(right):

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/02/01 19:26:47, paulmiller wrote:
> Does this file need its own directory?

Well, I want it to be reachable from the glue-layer (WebViewProviderFactory) and
from the crash-code.
We can just put it in the crash-directory - it just seemed a bit weird to put a
CommandLineUtil in the crash-directory ;) (and in general I prefer splitting
different code parts into sub-parts/directories to make it easier to
move/decouple from the rest of the code).
I don't mind too much either way.

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:36:
if (CommandLineUtil.isBuildDebuggable()) {
On 2017/02/01 19:26:47, paulmiller wrote:
> Isn't "CommandLineUtil." unnecessary?

Done.

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:39:
private volatile boolean mPermittedByUser = false;
On 2017/02/01 19:26:48, paulmiller wrote:
> mCancelUpload is protected with a lock, but mPermittedByUser is volatile with
no
> lock. Is there a reason for the difference?

Good point ;), mCancelUpload can be volatile as well (I'm adding Torne as
reviewer just to ensure I'm not shooting myself in the foot regarding volatile
here - IIUC volatile is fine to use as long as we only perform single operations
on a primitive variable - i.e. setting 'mPremittedByUser = true' is fine, but
doing compound operations like 'x++' is not ok).

https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:183:
PlatformServiceBridge.getInstance(mContext).queryMetricsSetting(
On 2017/02/01 19:26:48, paulmiller wrote:
> On 2017/02/01 17:40:48, gsennton wrote:
> > I'd like some feedback here - do you guys think we should bypass the whole
> > GmsCore query if the enabledForTesting command-line flag is ON? The reason
we
> > would do so is to avoid the potential issue of GmsCore never calling back to
> us
> > (but only in the case where enabled-for-testing is turned on). 
> 
> That would make sense. Not important though, given 1) GmsCore really should
call
> back and 2) enabled-for-testing isn't a configuration for use out in the wild.

Alright, thanks Paul. Leaving this as it is :)

[gsennton, 2017-02-02 14:30:13]

On 2017/02/02 14:29:43, gsennton wrote:
> Alright, ptal again :)
> Adding Torne just for the use of volatile, to ensure I'm not shooting myself
in
> the foot ;)
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
> File
>
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java
> (right):
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:1:
> // Copyright 2017 The Chromium Authors. All rights reserved.
> On 2017/02/01 19:26:47, paulmiller wrote:
> > Does this file need its own directory?
> 
> Well, I want it to be reachable from the glue-layer (WebViewProviderFactory)
and
> from the crash-code.
> We can just put it in the crash-directory - it just seemed a bit weird to put
a
> CommandLineUtil in the crash-directory ;) (and in general I prefer splitting
> different code parts into sub-parts/directories to make it easier to
> move/decouple from the rest of the code).
> I don't mind too much either way.
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/command_line/CommandLineUtil.java:36:
> if (CommandLineUtil.isBuildDebuggable()) {
> On 2017/02/01 19:26:47, paulmiller wrote:
> > Isn't "CommandLineUtil." unnecessary?
> 
> Done.
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
> File
>
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
> (right):
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:39:
> private volatile boolean mPermittedByUser = false;
> On 2017/02/01 19:26:48, paulmiller wrote:
> > mCancelUpload is protected with a lock, but mPermittedByUser is volatile
with
> no
> > lock. Is there a reason for the difference?
> 
> Good point ;), mCancelUpload can be volatile as well (I'm adding Torne as
> reviewer just to ensure I'm not shooting myself in the foot regarding volatile
> here - IIUC volatile is fine to use as long as we only perform single
operations
> on a primitive variable - i.e. setting 'mPremittedByUser = true' is fine, but
> doing compound operations like 'x++' is not ok).
> 
>
https://codereview.chromium.org/2655023008/diff/20001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:183:
> PlatformServiceBridge.getInstance(mContext).queryMetricsSetting(
> On 2017/02/01 19:26:48, paulmiller wrote:
> > On 2017/02/01 17:40:48, gsennton wrote:
> > > I'd like some feedback here - do you guys think we should bypass the whole
> > > GmsCore query if the enabledForTesting command-line flag is ON? The reason
> we
> > > would do so is to avoid the potential issue of GmsCore never calling back
to
> > us
> > > (but only in the case where enabled-for-testing is turned on). 
> > 
> > That would make sense. Not important though, given 1) GmsCore really should
> call
> > back and 2) enabled-for-testing isn't a configuration for use out in the
wild.
> 
> Alright, thanks Paul. Leaving this as it is :)

(volatile is used in MinidumpUploaderImpl)

[gsennton, 2017-02-02 15:35:02]

Talked to Torne - we don't need to use volatile or any specific synchronization
mechanism here at all - we are just setting a single boolean to true, and don't
care what earlier changes from other threads are visible to the thread reading
that boolean.

Filed crbug.com/687993 for a related potential threading issue that Torne
pointed out while discussing the code ;)

[paulmiller, 2017-02-02 22:47:07]

lgtm

https://codereview.chromium.org/2655023008/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
(right):

https://codereview.chromium.org/2655023008/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:144:
// correctly?
I think the assertOnUiThread you put in queryMetricsSetting pretty much covers
it. Not sure what other constraints there are to enforce.

[gsennton, 2017-02-03 13:53:16]

Thanks for the review Paul, I guess I need one from Selim as well :)

https://codereview.chromium.org/2655023008/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
(right):

https://codereview.chromium.org/2655023008/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:144:
// correctly?
On 2017/02/02 22:47:07, paulmiller wrote:
> I think the assertOnUiThread you put in queryMetricsSetting pretty much covers
> it. Not sure what other constraints there are to enforce.

Yeah, I added a couple of tests in PS5 to ensure that MinidumpUploaderImpl does
what we want it to depending on whether PlatformServicesBridge returns true :)
(upload minidumps vs. skipping them).

[sgurun-gerrit only, 2017-02-07 03:27:22]

lgtm

[gsennton, 2017-02-07 10:11:40]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-02-07 10:11:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulmiller@chromium.org
Link to the patchset: https://codereview.chromium.org/2655023008/#ps80001
(title: "Add tests to ensure return-value from PlatformServicesBridge is handled
correctly.")

[commit-bot: I haz the power, 2017-02-07 10:12:05]

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-07 10:14:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 10:14:06]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gsennton, 2017-02-07 10:16:45]

The CQ bit was unchecked by gsennton@chromium.org

[gsennton, 2017-02-07 10:24:08]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-02-07 10:24:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulmiller@chromium.org, sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/2655023008/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-07 10:24:34]

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-07 11:35:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486463048342740,
"parent_rev": "9fc90612e0bf684874654a270183e4a07069a2cf", "commit_rev":
"30d6e25b2f1c69090883e97110877815f1ae3c72"}

[commit-bot: I haz the power, 2017-02-07 11:35:51]

Description was changed from

==========
[Android WebView] Ensure Android Checkbox is on when uploading minidumps

Since minidump uploading can happen long after we copy minidumps to
WebView's separate minidump uploading service (and thus long after our
initial check to ensure the Android Checkbox is toggled on), the user
could have toggled the Checkbox off by the time we upload minidumps. We
now ensure the Checkbox is toggled on just before we upload the
minidumps.

This check can be overridden on debuggable builds through the
enable-crash-reporter-for-testing command line flag.

BUG=685576
==========

to

==========
[Android WebView] Ensure Android Checkbox is on when uploading minidumps

Since minidump uploading can happen long after we copy minidumps to
WebView's separate minidump uploading service (and thus long after our
initial check to ensure the Android Checkbox is toggled on), the user
could have toggled the Checkbox off by the time we upload minidumps. We
now ensure the Checkbox is toggled on just before we upload the
minidumps.

This check can be overridden on debuggable builds through the
enable-crash-reporter-for-testing command line flag.

BUG=685576

Review-Url: https://codereview.chromium.org/2655023008
Cr-Commit-Position: refs/heads/master@{#448600}
Committed:
https://chromium.googlesource.com/chromium/src/+/30d6e25b2f1c69090883e9711087...
==========

[commit-bot: I haz the power, 2017-02-07 11:35:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/30d6e25b2f1c69090883e9711087...