Webapp share dialog is not closed when client end of the connection is closed

Webapp share dialog is not closed when client end of the connection is closed

This problem is occurring because the share request dialog must be accepted
before the connection is considered authenticated and the host is only notified
of a disconnect if the connection is authenticated.  The host does receive
an access denied callback in this case as OnSessionAuthenticationFailed is
called.

The initial state (session is not authenticated until dialog is accepted) is
correct so I am adding code to dismiss the confirmation dialog if we receive
OnAccessDenied (via OnSessionAuthenticationFailed) for the JID represented by
the confirmation dialog.  I also reset the host state (from 'connecting' to
'received access code' (i.e. ready for client connection) when this occurs so
the webapp will update its UI appropriately.  This new behavior allows the
remote user to cancel while the share dialog is present and attempt to
reconnect using the same access code.  Since it is possible to display multiple
confirmation dialogs in one session, I removed the static Create method from
the confirmation dialog class and replaced it with a factory class.

On platforms where the confirmation dialog is displayed with a blocking UI
thread (i.e. Windows), the webapp UI will be updated once the timeout
timer fires or the user makes a selection.  On other platforms the dialog
will be dismissed automatically.

BUG=681159
Review-Url: https://codereview.chromium.org/2650443002
Cr-Commit-Position: refs/heads/master@{#458096}
Committed: https://chromium.googlesource.com/chromium/src/+/d15dc6a9d267c09c67d36838cba240d69bc68ab2

[joedow, 2017-01-20 18:54:43]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 18:55:00]

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

[joedow, 2017-01-20 19:30:42]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2017-01-20 19:30:43]

PTAL!

Thanks!
Joe

[commit-bot: I haz the power, 2017-01-20 19:37:14]

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

[commit-bot: I haz the power, 2017-01-20 19:37:15]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-01-20 20:05:44]

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:36: void Cancel();
I don't think we really need this method. The caller can just delete the proxy
to hide the dialog. It also knows if the callback has been called. Also calling
callback(CANCEL) doesn't seem like the right thing to do: CANCEL indicates that
the user clicked "Cancel" button, which is not the case here.

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:259: } else if (confirmation_dialog_proxy_) {
Can we just delete ConfirmationDialogProxy? ( i.e.
confirmation_dialog_proxy_.reset(); ). 
I would also put it above line 256.

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:260: confirmation_dialog_proxy_->Cancel();
If I understand correctly this will call OnConfirmationResult(CANCEL), which
will then call DisconnectOnNetworkThread(). I'm not sure we want this behavior.
If user tries to connect, then cancels, then tries to connect again we should
let them connect again instead of shutting down the host.


Also note the host allows multiple concurrent connections in authenticating
state, so potentially OnAccessDenied() can be called for a connection/jid that's
different from the one for which the current confirmation_dialog_proxy_ was
created. It looks already broken in the current host, I've filed this bug:
crbug.com/683317
I don't think we care too much about that scenario: just failing both
connections may be appropriate, but we certainly want to avoid crashes. Since
Cancel() is asynchronous, it makes it harder to make sure this scenario is
handled properly.

[joedow, 2017-03-14 16:23:12]

Description was changed from

==========
Webapp share dialog is not closed when client end of the connection is closed

This problem is occurring because the share request dialog must be accepted
before the connection is considered authenticated and the host is only notified
of a disconnect if the connection is authenticated.  The host does receive
an access denied callback in this case as OnSessionAuthenticationFailed is
called.

I think the initial state (session is not authenticated until dialog is
accepted) is correct so I am adding code to dismiss the confirmation dialog
if we receive OnAccessDenied (via OnSessionAuthenticationFailed).  I am
adding a Cancel() method on the dialog proxy which will post a task to cancel
the dialog.  This needs to be async as we are signalled from the host and we
don't want to trigger a synchronous teardown of the host from this method.

On platforms where the confirmation dialog is displayed with a blocking UI
thread (i.e. Windows), this disconnect will be processed once the timeout
timer fires or the user makes a selection.  On other platforms the dialog
will be dismissed automatically.

BUG=681159
==========

to

==========
Webapp share dialog is not closed when client end of the connection is closed

This problem is occurring because the share request dialog must be accepted
before the connection is considered authenticated and the host is only notified
of a disconnect if the connection is authenticated.  The host does receive
an access denied callback in this case as OnSessionAuthenticationFailed is
called.

The initial state (session is not authenticated until dialog is accepted) is
correct so I am adding code to dismiss the confirmation dialog if we receive
OnAccessDenied (via OnSessionAuthenticationFailed) for the JID represented by
the confirmation dialog.  I also reset the host state (from 'connecting' to
'received access code' (i.e. ready for client connection) when this occurs so
the webapp will update its UI appropriately.  This new behavior allows the
remote user to cancel while the share dialog is present and attempt to
reconnect using the same access code.  Since it is possible to display multiple
confirmation dialogs in one session, I removed the static Create method from
the confirmation dialog class and replaced it with a factory class.

On platforms where the confirmation dialog is displayed with a blocking UI
thread (i.e. Windows), the webapp UI will be updated once the timeout
timer fires or the user makes a selection.  On other platforms the dialog
will be dismissed automatically.

BUG=681159
==========

[joedow, 2017-03-14 16:28:34]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:29:08]

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

[joedow, 2017-03-14 16:46:36]

PTAL!

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:36: void Cancel();
On 2017/01/20 20:05:43, Sergey Ulanov wrote:
> I don't think we really need this method. The caller can just delete the proxy
> to hide the dialog. It also knows if the callback has been called. Also
calling
> callback(CANCEL) doesn't seem like the right thing to do: CANCEL indicates
that
> the user clicked "Cancel" button, which is not the case here.

Done.

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:259: } else if (confirmation_dialog_proxy_) {
On 2017/01/20 20:05:43, Sergey Ulanov wrote:
> Can we just delete ConfirmationDialogProxy? ( i.e.
> confirmation_dialog_proxy_.reset(); ). 
> I would also put it above line 256.

Acknowledged.

https://codereview.chromium.org/2650443002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:260: confirmation_dialog_proxy_->Cancel();
On 2017/01/20 20:05:43, Sergey Ulanov wrote:
> If I understand correctly this will call OnConfirmationResult(CANCEL), which
> will then call DisconnectOnNetworkThread(). I'm not sure we want this
behavior.
> If user tries to connect, then cancels, then tries to connect again we should
> let them connect again instead of shutting down the host.
> 
> 
> Also note the host allows multiple concurrent connections in authenticating
> state, so potentially OnAccessDenied() can be called for a connection/jid
that's
> different from the one for which the current confirmation_dialog_proxy_ was
> created. It looks already broken in the current host, I've filed this bug:
> crbug.com/683317
> I don't think we care too much about that scenario: just failing both
> connections may be appropriate, but we certainly want to avoid crashes. Since
> Cancel() is asynchronous, it makes it harder to make sure this scenario is
> handled properly.

resetting the proxy will prevent the dialog result from being returned to the
webapp which is what we want.  I think we should reset the connection state as
well here to allow for a reconnect attempt.

[commit-bot: I haz the power, 2017-03-14 17:08:48]

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

[commit-bot: I haz the power, 2017-03-14 17:08:49]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-03-14 21:50:36]

lgtm

https://codereview.chromium.org/2650443002/diff/60001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc (right):

https://codereview.chromium.org/2650443002/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc:126:
std::unique_ptr<StubIt2MeConfirmationDialog> dialog(
auto dialog = base::MakeUnique<StubIt2MeConfirmationDialog>(
    dialog_task_runner());

This is shorter and more readable.

[joedow, 2017-03-16 21:39:05]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[joedow, 2017-03-16 21:39:10]

Thanks!

https://codereview.chromium.org/2650443002/diff/60001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc (right):

https://codereview.chromium.org/2650443002/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc:126:
std::unique_ptr<StubIt2MeConfirmationDialog> dialog(
On 2017/03/14 21:50:35, Sergey Ulanov wrote:
> auto dialog = base::MakeUnique<StubIt2MeConfirmationDialog>(
>     dialog_task_runner());
> 
> This is shorter and more readable.

Done.

[commit-bot: I haz the power, 2017-03-16 21:39:34]

Dry run: 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-16 22:15:16]

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

[commit-bot: I haz the power, 2017-03-16 22:15:17]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-03-20 16:11:38]

The CQ bit was checked by joedow@chromium.org

[joedow, 2017-03-20 16:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2650443002/#ps120001
(title: "Merging changes from dependent CL")

[commit-bot: I haz the power, 2017-03-20 16: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-03-20 16:52:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490026298921870,
"parent_rev": "0256b97f24a3a7bc58cef9f291db1e3de2528f2a", "commit_rev":
"d15dc6a9d267c09c67d36838cba240d69bc68ab2"}

[commit-bot: I haz the power, 2017-03-20 16:54:28]

Description was changed from

==========
Webapp share dialog is not closed when client end of the connection is closed

This problem is occurring because the share request dialog must be accepted
before the connection is considered authenticated and the host is only notified
of a disconnect if the connection is authenticated.  The host does receive
an access denied callback in this case as OnSessionAuthenticationFailed is
called.

The initial state (session is not authenticated until dialog is accepted) is
correct so I am adding code to dismiss the confirmation dialog if we receive
OnAccessDenied (via OnSessionAuthenticationFailed) for the JID represented by
the confirmation dialog.  I also reset the host state (from 'connecting' to
'received access code' (i.e. ready for client connection) when this occurs so
the webapp will update its UI appropriately.  This new behavior allows the
remote user to cancel while the share dialog is present and attempt to
reconnect using the same access code.  Since it is possible to display multiple
confirmation dialogs in one session, I removed the static Create method from
the confirmation dialog class and replaced it with a factory class.

On platforms where the confirmation dialog is displayed with a blocking UI
thread (i.e. Windows), the webapp UI will be updated once the timeout
timer fires or the user makes a selection.  On other platforms the dialog
will be dismissed automatically.

BUG=681159
==========

to

==========
Webapp share dialog is not closed when client end of the connection is closed

This problem is occurring because the share request dialog must be accepted
before the connection is considered authenticated and the host is only notified
of a disconnect if the connection is authenticated.  The host does receive
an access denied callback in this case as OnSessionAuthenticationFailed is
called.

The initial state (session is not authenticated until dialog is accepted) is
correct so I am adding code to dismiss the confirmation dialog if we receive
OnAccessDenied (via OnSessionAuthenticationFailed) for the JID represented by
the confirmation dialog.  I also reset the host state (from 'connecting' to
'received access code' (i.e. ready for client connection) when this occurs so
the webapp will update its UI appropriately.  This new behavior allows the
remote user to cancel while the share dialog is present and attempt to
reconnect using the same access code.  Since it is possible to display multiple
confirmation dialogs in one session, I removed the static Create method from
the confirmation dialog class and replaced it with a factory class.

On platforms where the confirmation dialog is displayed with a blocking UI
thread (i.e. Windows), the webapp UI will be updated once the timeout
timer fires or the user makes a selection.  On other platforms the dialog
will be dismissed automatically.

BUG=681159

Review-Url: https://codereview.chromium.org/2650443002
Cr-Commit-Position: refs/heads/master@{#458096}
Committed:
https://chromium.googlesource.com/chromium/src/+/d15dc6a9d267c09c67d36838cba2...
==========

[commit-bot: I haz the power, 2017-03-20 16:54:29]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d15dc6a9d267c09c67d36838cba2...