[CRD iOS] Fix crash on SetSurfaceSize(int, int)

We got quite a few crash reports on
DesktopViewport::SetSurfaceSize(int, int). My suspicion is that the
connection is dropped when the keyboard is being shown/hidden so
animateSurfaceSize: is called when gestureInterpreter is already a
nullptr, causing a segfault. This is very hard to repro though since
the connection must be dropped (by killing the host?) during the
animation and the animation must tick one frame before the view closes
itself.

This CL fixes this by making both interpreters have the same lifetime
as RemotingClient and use SetContext() to enable/disable them.

Bug: 828204
Change-Id: I41c14ae8bc1d6edce8f3c32eb958256809821425
Reviewed-on: https://chromium-review.googlesource.com/1000096
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548969}

remoting/client/gesture_interpreter.cc

@@ -19,26 +19,31 @@ const float kScrollFlingTimeConstant = 250.f;
 }  // namespace
 
 namespace remoting {
-GestureInterpreter::GestureInterpreter(RendererProxy* renderer,
-                                       ChromotingSession* input_stub)
-    : renderer_(renderer),
-      input_stub_(input_stub),
-      pan_animation_(kOneFingerFlingTimeConstant,
+GestureInterpreter::GestureInterpreter()
+    // TODO(yuweih): These animations are better to take GetWeakPtr().
+    : pan_animation_(kOneFingerFlingTimeConstant,
                      base::Bind(&GestureInterpreter::PanWithoutAbortAnimations,
                                 base::Unretained(this))),
       scroll_animation_(
           kScrollFlingTimeConstant,
           base::Bind(&GestureInterpreter::ScrollWithoutAbortAnimations,
                      base::Unretained(this))),
-      weak_factory_(this) {
-  viewport_.RegisterOnTransformationChangedCallback(
-      base::Bind(&RendererProxy::SetTransformation,
-                 base::Unretained(renderer_)),
-      true);
-}
+      weak_factory_(this) {}
 
 GestureInterpreter::~GestureInterpreter() = default;
 
+void GestureInterpreter::SetContext(RendererProxy* renderer,
+                                    ChromotingSession* input_stub) {
+  renderer_ = renderer;
+  input_stub_ = input_stub;
+  auto transformation_callback =
+      renderer_ ? base::BindRepeating(&RendererProxy::SetTransformation,
+                                      base::Unretained(renderer_))
+                : DesktopViewport::TransformationCallback();
+  viewport_.RegisterOnTransformationChangedCallback(transformation_callback,
+                                                    true);
+}
+
 void GestureInterpreter::SetInputMode(InputMode mode) {
   switch (mode) {
     case DIRECT_INPUT_MODE:
@@ -51,6 +56,9 @@ void GestureInterpreter::SetInputMode(InputMode mode) {
       NOTREACHED();
   }
   input_mode_ = mode;
+  if (!renderer_) {
+    return;
+  }
   renderer_->SetCursorVisibility(input_strategy_->IsCursorVisible());
   ViewMatrix::Point cursor_position = input_strategy_->GetCursorPosition();
   renderer_->SetCursorPosition(cursor_position.x, cursor_position.y);
@@ -100,7 +108,7 @@ void GestureInterpreter::Drag(float x, float y, GestureState state) {
   bool is_dragging_mode = state != GESTURE_ENDED;
   SetGestureInProgress(TouchInputStrategy::DRAG, is_dragging_mode);
 
-  if (!viewport_.IsViewportReady() ||
+  if (!input_stub_ || !viewport_.IsViewportReady() ||
       !input_strategy_->TrackTouchInput({x, y}, viewport_)) {
     return;
   }
@@ -191,17 +199,22 @@ void GestureInterpreter::PanWithoutAbortAnimations(float translation_x,
       // Drag() will inject the position so don't need to do that in that case.
       InjectCursorPosition(cursor_position.x, cursor_position.y);
     }
-    renderer_->SetCursorPosition(cursor_position.x, cursor_position.y);
+    if (renderer_) {
+      renderer_->SetCursorPosition(cursor_position.x, cursor_position.y);
+    }
   }
 }
 
 void GestureInterpreter::InjectCursorPosition(float x, float y) {
+  if (!input_stub_) {
+    return;
+  }
   input_stub_->SendMouseEvent(
       x, y, protocol::MouseEvent_MouseButton_BUTTON_UNDEFINED, false);
 }
 
 void GestureInterpreter::ScrollWithoutAbortAnimations(float dx, float dy) {
-  if (!viewport_.IsViewportReady()) {
+  if (!input_stub_ || !viewport_.IsViewportReady()) {
     return;
   }
   ViewMatrix::Point desktopDelta =
@@ -218,7 +231,7 @@ void GestureInterpreter::InjectMouseClick(
     float touch_x,
     float touch_y,
     protocol::MouseEvent_MouseButton button) {
-  if (!viewport_.IsViewportReady() ||
+  if (!input_stub_ || !viewport_.IsViewportReady() ||
       !input_strategy_->TrackTouchInput({touch_x, touch_y}, viewport_)) {
     return;
   }
@@ -254,7 +267,9 @@ void GestureInterpreter::StartInputFeedback(
     // the *2 logic inside the renderer.
     float diameter_on_desktop =
         2.f * feedback_radius / viewport_.GetTransformation().GetScale();
-    renderer_->StartInputFeedback(cursor_x, cursor_y, diameter_on_desktop);
+    if (renderer_) {
+      renderer_->StartInputFeedback(cursor_x, cursor_y, diameter_on_desktop);
+    }
   }
 }
 

remoting/client/gesture_interpreter.h

@@ -31,9 +31,13 @@ class GestureInterpreter {
     TRACKPAD_INPUT_MODE
   };
 
-  GestureInterpreter(RendererProxy* renderer, ChromotingSession* input_stub);
+  GestureInterpreter();
   ~GestureInterpreter();
 
+  // Sets the context for the interpreter. Both arguments are nullable. If both
+  // are nullptr then methods below will have no effect.
+  void SetContext(RendererProxy* renderer, ChromotingSession* input_stub);
+
   // Must be called right after the renderer is ready.
   void SetInputMode(InputMode mode);
 
@@ -107,8 +111,8 @@ class GestureInterpreter {
   InputMode input_mode_ = UNDEFINED_INPUT_MODE;
   std::unique_ptr<TouchInputStrategy> input_strategy_;
   DesktopViewport viewport_;
-  RendererProxy* renderer_;
-  ChromotingSession* input_stub_;
+  RendererProxy* renderer_ = nullptr;
+  ChromotingSession* input_stub_ = nullptr;
   TouchInputStrategy::Gesture gesture_in_progress_;
 
   FlingAnimation pan_animation_;

remoting/client/input/keyboard_interpreter.cc

@@ -10,19 +10,34 @@
 
 namespace remoting {
 
-KeyboardInterpreter::KeyboardInterpreter(ClientInputInjector* input_injector) {
-  // TODO(nicholss): This should be configurable.
-  input_strategy_.reset(new TextKeyboardInputStrategy(input_injector));
-}
+KeyboardInterpreter::KeyboardInterpreter() = default;
 
 KeyboardInterpreter::~KeyboardInterpreter() = default;
 
+void KeyboardInterpreter::SetContext(ClientInputInjector* input_injector) {
+  // TODO(nicholss): This should be configurable.
+  if (input_injector) {
+    input_strategy_ =
+        std::make_unique<TextKeyboardInputStrategy>(input_injector);
+  } else {
+    input_strategy_.reset();
+  }
+}
+
 void KeyboardInterpreter::HandleTextEvent(const std::string& text,
                                           uint8_t modifiers) {
+  if (!input_strategy_) {
+    return;
+  }
+
   input_strategy_->HandleTextEvent(text, modifiers);
 }
 
 void KeyboardInterpreter::HandleDeleteEvent(uint8_t modifiers) {
+  if (!input_strategy_) {
+    return;
+  }
+
   base::queue<KeyEvent> keys;
   // TODO(nicholss): Handle modifers.
   // Key press.
@@ -35,6 +50,10 @@ void KeyboardInterpreter::HandleDeleteEvent(uint8_t modifiers) {
 }
 
 void KeyboardInterpreter::HandleCtrlAltDeleteEvent() {
+  if (!input_strategy_) {
+    return;
+  }
+
   base::queue<KeyEvent> keys;
 
   // Key press.
@@ -51,6 +70,10 @@ void KeyboardInterpreter::HandleCtrlAltDeleteEvent() {
 }
 
 void KeyboardInterpreter::HandlePrintScreenEvent() {
+  if (!input_strategy_) {
+    return;
+  }
+
   base::queue<KeyEvent> keys;
 
   // Key press.

remoting/client/input/keyboard_interpreter.h

@@ -19,9 +19,12 @@ class ClientInputInjector;
 // handling of text events to the selected keyboard input strategy.
 class KeyboardInterpreter {
  public:
-  explicit KeyboardInterpreter(ClientInputInjector* input_injector);
+  explicit KeyboardInterpreter();
   ~KeyboardInterpreter();
 
+  // If |input_injector| is nullptr, all methods below will have no effect.
+  void SetContext(ClientInputInjector* input_injector);
+
   // Delegates to |KeyboardInputStrategy| to covert and send the input.
   void HandleTextEvent(const std::string& text, uint8_t modifiers);
   // Delegates to |KeyboardInputStrategy| to covert and send the delete.

remoting/client/ui/desktop_viewport.cc

@@ -117,8 +117,8 @@ void DesktopViewport::RegisterOnTransformationChangedCallback(
     const TransformationCallback& callback,
     bool run_immediately) {
   on_transformation_changed_ = callback;
-  if (IsViewportReady() && run_immediately) {
-    callback.Run(desktop_to_surface_transform_);
+  if (on_transformation_changed_ && IsViewportReady() && run_immediately) {
+    on_transformation_changed_.Run(desktop_to_surface_transform_);
   }
 }
 

remoting/ios/app/host_view_controller.mm

@@ -466,6 +466,8 @@ - (void)applyInputMode {
   }
 }
 
+// TODO(yuweih): This method is badly named. Should be changed to
+// "didTapShowMenu".
 - (void)didTap:(id)sender {
   // TODO(nicholss): The FAB is being used to launch an alert window with
   // more options. This is not ideal but it gets us an easy way to make a

remoting/ios/session/remoting_client.h

@@ -95,12 +95,11 @@ fetchSecretWithPairingSupported:(BOOL)pairingSupported
 @property(nonatomic, strong) GlDisplayHandler* displayHandler;
 // The host info used to make the remoting client connection.
 @property(nonatomic, readonly) HostInfo* hostInfo;
-// The gesture interpreter used to handle gestures.
-// This is valid only after the client has connected to the host. Always use
-// RemotingClient.gestureInterpreter instead of storing the pointer separately.
+// The gesture interpreter used to handle gestures. It has no effect if the
+// session is not connected.
 @property(nonatomic, readonly) remoting::GestureInterpreter* gestureInterpreter;
 // The keyboard interpreter used to convert key events and send them to the
-// host.
+// host. It has no effect if the session is not connected.
 @property(nonatomic, readonly)
     remoting::KeyboardInterpreter* keyboardInterpreter;
 

remoting/ios/session/remoting_client.mm

@@ -65,9 +65,9 @@ @interface RemotingClient () {
   std::unique_ptr<remoting::RemotingClientSessonDelegate> _sessonDelegate;
   ClientSessionDetails* _sessionDetails;
   remoting::protocol::SecretFetchedCallback _secretFetchedCallback;
+  remoting::GestureInterpreter _gestureInterpreter;
+  remoting::KeyboardInterpreter _keyboardInterpreter;
   std::unique_ptr<remoting::RendererProxy> _renderer;
-  std::unique_ptr<remoting::GestureInterpreter> _gestureInterpreter;
-  std::unique_ptr<remoting::KeyboardInterpreter> _keyboardInterpreter;
   std::unique_ptr<remoting::AudioPlayerIos> _audioPlayer;
   std::unique_ptr<remoting::ChromotingSession> _session;
 }
@@ -141,9 +141,8 @@ - (void)connectToHost:(HostInfo*)hostInfo
       [_displayHandler CreateVideoRenderer],
       _audioPlayer->GetAudioStreamConsumer(), info));
   _renderer = [_displayHandler CreateRendererProxy];
-  _gestureInterpreter.reset(
-      new remoting::GestureInterpreter(_renderer.get(), _session.get()));
-  _keyboardInterpreter.reset(new remoting::KeyboardInterpreter(_session.get()));
+  _gestureInterpreter.SetContext(_renderer.get(), _session.get());
+  _keyboardInterpreter.SetContext(_session.get());
 
   _session->Connect();
   _audioPlayer->Start();
@@ -167,8 +166,8 @@ - (void)disconnectFromHost {
     _runtime->display_task_runner()->DeleteSoon(FROM_HERE, _renderer.release());
   }
 
-  _gestureInterpreter.reset();
-  _keyboardInterpreter.reset();
+  _gestureInterpreter.SetContext(nullptr, nullptr);
+  _keyboardInterpreter.SetContext(nullptr);
 }
 
 #pragma mark - Eventing
@@ -195,11 +194,11 @@ - (HostInfo*)hostInfo {
 }
 
 - (remoting::GestureInterpreter*)gestureInterpreter {
-  return _gestureInterpreter.get();
+  return &_gestureInterpreter;
 }
 
 - (remoting::KeyboardInterpreter*)keyboardInterpreter {
-  return _keyboardInterpreter.get();
+  return &_keyboardInterpreter;
 }
 
 #pragma mark - ChromotingSession::Delegate
@@ -370,15 +369,11 @@ - (void)createFeedbackDataWithCallback:
 #pragma mark - GlDisplayHandlerDelegate
 
 - (void)canvasSizeChanged:(CGSize)size {
-  if (_gestureInterpreter) {
-    _gestureInterpreter->OnDesktopSizeChanged(size.width, size.height);
-  }
+  _gestureInterpreter.OnDesktopSizeChanged(size.width, size.height);
 }
 
 - (void)rendererTicked {
-  if (_gestureInterpreter) {
-    _gestureInterpreter->ProcessAnimations();
-  }
+  _gestureInterpreter.ProcessAnimations();
 }
 
 @end