[remoting][FTL] Only delay signal strategy destruction in mobile clients

FtlSignalStrategy deletes underlying messaging client and other members
with a 5-second delay to allow session-terminate message to be sent
after deleting the signal strategy. The current implementation doesn't
work well at shutdown. When the sequence itself is being deleted, all
pending tasks will be deleted on an arbitrary thread. This will cause
a race condition in gRPC executor since it tries to delete unique_ptrs
bound into the closure.

Unlike CL 1692736 This CL provides a different way to solve this issue.
Currently only mobile clients use short-lived FtlSignalStrategy, so this
CL moves the delay destruction logic into ChromotingSession. Mobile
clients don't do any cleanup at shutdown, so they won't experience crash
at shutdown even if the app gets killed right after a session is
terminated.

ChromeOS enterprise IT2ME host still has a risk of session-terminate
not being sent at shutdown, but this is probably fine since the host
can't initiate a session termination (i.e. no disconnect window) AFAIK.

Bug: 979398
Change-Id: Ie12f48257e6f9c59391a7ed15224987ffdc879ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1707592
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678576}

remoting/client/chromoting_session.cc

@@ -54,6 +54,11 @@ const int kMinDimension = 640;
 // Interval at which to log performance statistics, if enabled.
 constexpr base::TimeDelta kPerfStatsInterval = base::TimeDelta::FromMinutes(1);
 
+// Delay to destroy the signal strategy, so that the session-terminate event can
+// still be sent out.
+constexpr base::TimeDelta kDestroySignalingDelay =
+    base::TimeDelta::FromSeconds(2);
+
 bool IsClientResolutionValid(int dips_width, int dips_height) {
   // This prevents sending resolution on a portrait mode small phone screen
   // because resizing the remote desktop to portrait will mess with icons and
@@ -451,15 +456,31 @@ base::WeakPtr<ChromotingSession::Core> ChromotingSession::Core::GetWeakPtr() {
 }
 
 void ChromotingSession::Core::Invalidate() {
+  DCHECK(network_task_runner()->BelongsToCurrentThread());
+
   // Prevent all pending and future calls from ChromotingSession.
   weak_factory_.InvalidateWeakPtrs();
 
   client_.reset();
   token_getter_.reset();
-  signaling_.reset();
   perf_tracker_.reset();
   client_context_.reset();
   session_context_.reset();
+
+  // Dirty hack to make sure session-terminate message is sent before
+  // |signaling_| gets deleted. W/o the message being sent, the other side will
+  // believe an error has occurred.
+  if (signaling_) {
+    signaling_->Disconnect();
+    network_task_runner()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(
+            [](std::unique_ptr<SignalStrategy> signaling) {
+              signaling.reset();
+            },
+            std::move(signaling_)),
+        kDestroySignalingDelay);
+  }
 }
 
 void ChromotingSession::Core::ConnectOnNetworkThread() {

remoting/signaling/ftl_signal_strategy.cc

@@ -28,9 +28,6 @@ namespace remoting {
 
 namespace {
 
-constexpr base::TimeDelta kDestroyMessagingClientDelay =
-    base::TimeDelta::FromSeconds(5);
-
 SignalStrategy::Error GrpcStatusToSignalingError(const grpc::Status& status) {
   switch (status.error_code()) {
     case grpc::StatusCode::OK:
@@ -130,26 +127,6 @@ FtlSignalStrategy::Core::Core(
 FtlSignalStrategy::Core::~Core() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   Disconnect();
-
-  registration_manager_->SignOut();
-  // Dirty hack to make sure session-terminate message is sent before the
-  // |messaging_client_| gets deleted.
-  // TODO(yuweih): Either improve this by waiting for request queue to drain or
-  // make MessagingClient a singleton if we decide to reuse it for host
-  // status discovery.
-  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](std::unique_ptr<RegistrationManager> registration_manager,
-             std::unique_ptr<MessagingClient> messaging_client,
-             std::unique_ptr<OAuthTokenGetter> oauth_token_getter) {
-            messaging_client.reset();
-            registration_manager.reset();
-            oauth_token_getter.reset();
-          },
-          std::move(registration_manager_), std::move(messaging_client_),
-          std::move(oauth_token_getter_)),
-      kDestroyMessagingClientDelay);
 }
 
 void FtlSignalStrategy::Core::Connect() {

remoting/signaling/ftl_signal_strategy.h

@@ -26,6 +26,11 @@ class FtlSignalStrategy : public SignalStrategy {
   // send out pending requests after the instance is deleted.
   FtlSignalStrategy(std::unique_ptr<OAuthTokenGetter> oauth_token_getter,
                     std::unique_ptr<FtlDeviceIdProvider> device_id_provider);
+
+  // Note that pending outgoing messages will be silently dropped when the
+  // signal strategy is being deleted. If you want to send last minute messages,
+  // consider calling Disconnect() then posting a delayed task to delete the
+  // strategy.
   ~FtlSignalStrategy() override;
 
   // SignalStrategy interface.

remoting/test/ftl_signaling_playground.cc

@@ -20,6 +20,7 @@
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
+#include "base/time/time.h"
 #include "jingle/glue/thread_wrapper.h"
 #include "remoting/base/chromium_url_request.h"
 #include "remoting/base/grpc_support/grpc_async_unary_request.h"
@@ -60,6 +61,9 @@ constexpr char kSwitchNamePin[] = "pin";
 constexpr char kSwitchNameHostId[] = "host-id";
 constexpr char kSwitchNameUseChromotocol[] = "use-chromotocol";
 
+// Delay to allow sending session-terminate before tearing down.
+constexpr base::TimeDelta kTearDownDelay = base::TimeDelta::FromSeconds(2);
+
 const char* SignalStrategyErrorToString(SignalStrategy::Error error) {
   switch (error) {
     case SignalStrategy::OK:
@@ -342,11 +346,12 @@ void FtlSignalingPlayground::OnSessionStateChange(
       break;
   }
 
-  TearDownAndRunCallback();
+  AsyncTearDownAndRunCallback();
 }
 
 void FtlSignalingPlayground::AsyncTearDownAndRunCallback() {
-  tear_down_timer_.Start(FROM_HERE, base::TimeDelta(), this,
+  HOST_LOG << "Tearing down in " << kTearDownDelay;
+  tear_down_timer_.Start(FROM_HERE, kTearDownDelay, this,
                          &FtlSignalingPlayground::TearDownAndRunCallback);
 }