arc: Provide more logging for network failures.

arc: Provide more logging for network failures.

This add more loggin for network failures and handles network
timeouts as dedicated UMA entry.

BUG=697579
Test=Manually. Emulate network unavailble error. GMS Services error
     is shown. chrome:://histograms shows value 19 recorded. Update
     only ARC side and repeat the test to validate condition with
     mojo different versions. The same error is shown but UMA
     value 14 is recorded in UMA (legacy case)

Review-Url: https://codereview.chromium.org/2723263003
Cr-Commit-Position: refs/heads/master@{#454501}
Committed: https://chromium.googlesource.com/chromium/src/+/337f3e54b359b2b83950d4e61d5c6b05108bc630

[khmel, 2017-03-01 20:39:08]

khmel@chromium.org changed reviewers:
+ yusukes@chromium.org

[khmel, 2017-03-01 20:39:10]

Hi Yusuke,

PTAL

Thanks

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:66: [MinVersion=8] NO_NETWORK_CONNECTION = 15,
 [MinVersion=8] in enum is not sufficient to uprev host version. Dummy is added
to force this.

[Yusuke Sato, 2017-03-01 20:57:10]

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:202: void
ArcAuthService::Dummy() {
same, please move this up.

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.h (right):

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.h:44: void Dummy();
sort (see my comment in mojom)

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:66: [MinVersion=8] NO_NETWORK_CONNECTION = 15,
On 2017/03/01 20:39:09, khmel wrote:
>  [MinVersion=8] in enum is not sufficient to uprev host version. Dummy is
added
> to force this.

What happens if you send NO_NETWORK_CONNECTION to older (version=7) Chrome?

If either Chrome or the container crashes (or wildly misbehaves), adding Dummy()
seems fine, but if nothing fatal (other than NOTREACHED logs, for example)
happens, just adding the enum but not adding Dummy() might be okay.

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:137: [MinVersion=8] Dummy@9();
Please move this to line 123 to lexicographically sort methods (that's the whole
point for having @N :)

[Luis Héctor Chávez, 2017-03-01 21:00:05]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2017-03-01 21:00:06]

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:66: [MinVersion=8] NO_NETWORK_CONNECTION = 15,
On 2017/03/01 20:57:10, Yusuke Sato wrote:
> On 2017/03/01 20:39:09, khmel wrote:
> >  [MinVersion=8] in enum is not sufficient to uprev host version. Dummy is
> added
> > to force this.
> 
> What happens if you send NO_NETWORK_CONNECTION to older (version=7) Chrome?
> 
> If either Chrome or the container crashes (or wildly misbehaves), adding
Dummy()
> seems fine, but if nothing fatal (other than NOTREACHED logs, for example)
> happens, just adding the enum but not adding Dummy() might be okay.

I'm also slightly against adding the dummy function. Please file a bug in crbug/
to make the code generator take this into account (add yzshen@, but he might not
be able to take this work immediately. w.c.s. I'll get to it by end of next
week).

[khmel, 2017-03-01 21:33:06]

Thanks for your quick feedback!

PTAL updated version

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2723263003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:202: void
ArcAuthService::Dummy() {
On 2017/03/01 20:57:09, Yusuke Sato wrote:
> same, please move this up.

Deprecated, thanks for catching this!

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:66: [MinVersion=8] NO_NETWORK_CONNECTION = 15,
On 2017/03/01 21:00:06, Luis Héctor Chávez wrote:
> On 2017/03/01 20:57:10, Yusuke Sato wrote:
> > On 2017/03/01 20:39:09, khmel wrote:
> > >  [MinVersion=8] in enum is not sufficient to uprev host version. Dummy is
> > added
> > > to force this.
> > 
> > What happens if you send NO_NETWORK_CONNECTION to older (version=7) Chrome?
> > 
> > If either Chrome or the container crashes (or wildly misbehaves), adding
> Dummy()
> > seems fine, but if nothing fatal (other than NOTREACHED logs, for example)
> > happens, just adding the enum but not adding Dummy() might be okay.
> 
> I'm also slightly against adding the dummy function. Please file a bug in
crbug/
> to make the code generator take this into account (add yzshen@, but he might
not
> be able to take this work immediately. w.c.s. I'll get to it by end of next
> week).

Personally I also consider this as minor part but for 100% correctness needed to
do that trick.
Nothing dangerous happens in worse case when Android has updated version but
Chrome not. As I tested without dummy and tricky manipulation with mojo version
"Unknown error" is shown. So I removed dummy part. Also I will be landing Chrome
first and we should never face this case.

https://codereview.chromium.org/2723263003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:137: [MinVersion=8] Dummy@9();
On 2017/03/01 20:57:10, Yusuke Sato wrote:
> Please move this to line 123 to lexicographically sort methods (that's the
whole
> point for having @N :)

Good point, however deprecated :)

[Yusuke Sato, 2017-03-01 21:40:28]

arc/ lgtm

[khmel, 2017-03-01 21:43:39]

khmel@chromium.org changed reviewers:
+ dcheng@chromium.org, isherman@chromium.org

[khmel, 2017-03-01 21:43:40]

Thank you Yusuke for review!

Adding 

isherman@ - UMA
dcheng@ - mojom

PTAL

[Ilya Sherman, 2017-03-01 23:10:49]

histograms.xml lgtm

[Luis Héctor Chávez, 2017-03-01 23:17:09]

final nit. otherwise lgtm.

https://codereview.chromium.org/2723263003/diff/60001/components/arc/net/arc_...
File components/arc/net/arc_net_host_impl.cc (right):

https://codereview.chromium.org/2723263003/diff/60001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:58: VLOG(1) << "Required parameter " <<
key << " was not found.";
maybe NOTREACHED() << ... ?

[hidehiko, 2017-03-02 01:44:44]

hidehiko@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hidehiko, 2017-03-02 01:44:45]

Drive-by, lgtm.

https://codereview.chromium.org/2723263003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2723263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80004: +  <int value="19" label="Network
is unavailable"/>
nit: maybe comment "18" is intentionally skipped?

[dcheng, 2017-03-02 07:14:31]

mojo lgtm

[khmel, 2017-03-03 00:06:50]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-03 00:07:23]

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

[khmel, 2017-03-03 00:09:47]

Thank you for review!

https://codereview.chromium.org/2723263003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2723263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80004: +  <int value="19" label="Network
is unavailable"/>
On 2017/03/02 01:44:45, hidehiko wrote:
> nit: maybe comment "18" is intentionally skipped?

Good question. This is constant for SERVER_COMMUNICATION_ERROR which we report
differently for historical reason and should never come to this basked. Probably
it might have sense to add here to for consistency.

[commit-bot: I haz the power, 2017-03-03 01:09:55]

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

[commit-bot: I haz the power, 2017-03-03 01:09:56]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2017-03-03 01:46:06]

The CQ bit was checked by khmel@chromium.org

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

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-03-03 03:25:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488505566422710,
"parent_rev": "759ef7f0d55e443ac58ba3acd2d867d483713dfb", "commit_rev":
"337f3e54b359b2b83950d4e61d5c6b05108bc630"}

[commit-bot: I haz the power, 2017-03-03 03:26:12]

Description was changed from

==========
arc: Provide more logging for network failures.

This add more loggin for network failures and handles network
timeouts as dedicated UMA entry.

BUG=697579
Test=Manually. Emulate network unavailble error. GMS Services error
     is shown. chrome:://histograms shows value 19 recorded. Update
     only ARC side and repeat the test to validate condition with
     mojo different versions. The same error is shown but UMA
     value 14 is recorded in UMA (legacy case)
==========

to

==========
arc: Provide more logging for network failures.

This add more loggin for network failures and handles network
timeouts as dedicated UMA entry.

BUG=697579
Test=Manually. Emulate network unavailble error. GMS Services error
     is shown. chrome:://histograms shows value 19 recorded. Update
     only ARC side and repeat the test to validate condition with
     mojo different versions. The same error is shown but UMA
     value 14 is recorded in UMA (legacy case)

Review-Url: https://codereview.chromium.org/2723263003
Cr-Commit-Position: refs/heads/master@{#454501}
Committed:
https://chromium.googlesource.com/chromium/src/+/337f3e54b359b2b83950d4e61d5c...
==========

[commit-bot: I haz the power, 2017-03-03 03:26:14]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/337f3e54b359b2b83950d4e61d5c...