arc: Fix double provisioning failure reporting.

arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported.
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
Committed: https://crrev.com/42d24efdbeff42ab083b9d3b59ddb1a4753b370e
Cr-Commit-Position: refs/heads/master@{#436794}

[khmel, 2016-12-06 17:32:05]

Description was changed from

==========
arc: Fix double provisioning failure reporting.

In case error occurs, error is reported. In many cases this causes
Arc bridge stopping mojo closing. On mojo close Cancel expection
is reported to Arc provision step. It itself sends one more Unknown
error that overrides previous error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
==========

to

==========
arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported. 
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
==========

[khmel, 2016-12-06 17:32:05]

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

[khmel, 2016-12-06 17:32:14]

Hi Luis,

PTAL

[Luis Héctor Chávez, 2016-12-06 17:35:20]

Thanks, this is much cleaner!

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:197:
provisioning_error_reported_ = false;
Is this one needed? Shouldn't we just use the one in StartArc()?

[khmel, 2016-12-06 17:46:55]

PTAL

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:197:
provisioning_error_reported_ = false;
On 2016/12/06 17:35:20, Luis Héctor Chávez wrote:
> Is this one needed? Shouldn't we just use the one in StartArc()?

Actually not, more paranoiac...

[Luis Héctor Chávez, 2016-12-06 17:50:45]

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

https://codereview.chromium.org/2554743003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:249: DCHECK_EQ(state_,
State::ACTIVE);
Move this after L258, because the state won't be State::ACTIVE the next time
this is called.

And speaking of that, can you add a small unit test that exercises this code
path?

[hidehiko, 2016-12-06 18:10:52]

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

[hidehiko, 2016-12-06 18:10:53]

Thank you for offline discussion.
Drive-by.

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:252: if (result !=
ProvisioningResult::SUCCESS) {
Per offline discussion, we want to guarantee OnProvisioningFinished() exactly
once per ArcSession run.
So, could you make it so?
- Removing this if condition.
- s/provisioning_error_reported_/provisioining_finished_called_/ or something?
- at L254: I think as this is unexpected behavior, NOTREACHED() should be
better.

[khmel, 2016-12-06 18:13:35]

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:252: if (result !=
ProvisioningResult::SUCCESS) {
On 2016/12/06 18:10:53, hidehiko wrote:
> Per offline discussion, we want to guarantee OnProvisioningFinished() exactly
> once per ArcSession run.
> So, could you make it so?
> - Removing this if condition.
> - s/provisioning_error_reported_/provisioining_finished_called_/ or something?
> - at L254: I think as this is unexpected behavior, NOTREACHED() should be
> better.

Per our discussion I thought that we want to have something like
ExactlyOneCallback. So check is needed here.

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

https://codereview.chromium.org/2554743003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:249: DCHECK_EQ(state_,
State::ACTIVE);
On 2016/12/06 17:50:45, Luis Héctor Chávez wrote:
> Move this after L258, because the state won't be State::ACTIVE the next time
> this is called.
> 
> And speaking of that, can you add a small unit test that exercises this code
> path?

This code is dropped in next CL https://codereview.chromium.org/2541173002/. 
Anyway moved it here.

[hidehiko, 2016-12-06 18:15:54]

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:252: if (result !=
ProvisioningResult::SUCCESS) {
On 2016/12/06 18:13:35, khmel wrote:
> On 2016/12/06 18:10:53, hidehiko wrote:
> > Per offline discussion, we want to guarantee OnProvisioningFinished()
exactly
> > once per ArcSession run.
> > So, could you make it so?
> > - Removing this if condition.
> > - s/provisioning_error_reported_/provisioining_finished_called_/ or
something?
> > - at L254: I think as this is unexpected behavior, NOTREACHED() should be
> > better.
> 
> Per our discussion I thought that we want to have something like
> ExactlyOneCallback. So check is needed here.

I meant, it is not needed to check if result is SUCCESS or not.
If this is called or not is still needed, as you said.

[khmel, 2016-12-06 18:41:13]

PTAL

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:252: if (result !=
ProvisioningResult::SUCCESS) {
On 2016/12/06 18:15:54, hidehiko wrote:
> On 2016/12/06 18:13:35, khmel wrote:
> > On 2016/12/06 18:10:53, hidehiko wrote:
> > > Per offline discussion, we want to guarantee OnProvisioningFinished()
> exactly
> > > once per ArcSession run.
> > > So, could you make it so?
> > > - Removing this if condition.
> > > - s/provisioning_error_reported_/provisioining_finished_called_/ or
> something?
> > > - at L254: I think as this is unexpected behavior, NOTREACHED() should be
> > > better.
> > 
> > Per our discussion I thought that we want to have something like
> > ExactlyOneCallback. So check is needed here.
> 
> I meant, it is not needed to check if result is SUCCESS or not.
> If this is called or not is still needed, as you said.

Got you! However LOG(WARNING) sounds better because this is actual case and we
really handle it. Also added check for SUCCESS

[hidehiko, 2016-12-06 18:50:02]

lgtm. Defer to Luis.

https://codereview.chromium.org/2554743003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:254: LOG(WARNING) << "
Provisioning result was already reported. Ignoring "
I'm ok with the current code for the short term fix, but OnProvisioningFinished
twice or more still sounds like unexpected situation, regardless of whether it
is an error or not.
Could you add TODO to make it NOTREACHED() once you fix the container?

[Luis Héctor Chávez, 2016-12-06 22:09:34]

lgtm. thanks for adding the test!

[khmel, 2016-12-06 22:49:57]

Thank you for review and discussion that helps to reveal all edges of this
problem.

https://codereview.chromium.org/2554743003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2554743003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:254: LOG(WARNING) << "
Provisioning result was already reported. Ignoring "
On 2016/12/06 18:50:01, hidehiko wrote:
> I'm ok with the current code for the short term fix, but
OnProvisioningFinished
> twice or more still sounds like unexpected situation, regardless of whether it
> is an error or not.
> Could you add TODO to make it NOTREACHED() once you fix the container?

Done.

[khmel, 2016-12-06 22:50:01]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-12-06 22:50:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2554743003/#ps60001
(title: "rebase / todo added")

[commit-bot: I haz the power, 2016-12-06 22:50:44]

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, 2016-12-07 00:49:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481064601560100,
"parent_rev": "e3eba20507c09523306ddd7deaf29b1087afa95f", "commit_rev":
"68b930088e2f6fd3fe5dad9c94b7ae2493a69aa5"}

[commit-bot: I haz the power, 2016-12-07 00:50:04]

Description was changed from

==========
arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported. 
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
==========

to

==========
arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported. 
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
==========

[commit-bot: I haz the power, 2016-12-07 00:50:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-07 00:53:44]

Description was changed from

==========
arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported. 
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667
==========

to

==========
arc: Fix double provisioning failure reporting.

In case a auth code request failure occurs, error is reported.
In many cases this causes Arc bridge stop and mojo close. On mojo
close Cancel exception is reported to Arc provision step. It itself
sends one more Unknown error that overrides previous error. This CL
ignore second and next error.

TEST=Manually on device. Emulate server error and instead unknown
     error server communication error is shown.
BUG=b/33347895
BUG=671667

Committed: https://crrev.com/42d24efdbeff42ab083b9d3b59ddb1a4753b370e
Cr-Commit-Position: refs/heads/master@{#436794}
==========

[commit-bot: I haz the power, 2016-12-07 00:53:45]

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