From 2967489d082e4175e49956f5dd5b6d50468c5f73 Mon Sep 17 00:00:00 2001 From: Yuwei Huang Date: Fri, 6 Apr 2018 23:35:59 +0000 Subject: [PATCH] [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 Commit-Queue: Yuwei Huang Cr-Commit-Position: refs/heads/master@{#548969} --- remoting/client/gesture_interpreter.cc | 47 ++++++++++++------- remoting/client/gesture_interpreter.h | 10 ++-- remoting/client/input/keyboard_interpreter.cc | 31 ++++++++++-- remoting/client/input/keyboard_interpreter.h | 5 +- remoting/client/ui/desktop_viewport.cc | 4 +- remoting/ios/app/host_view_controller.mm | 2 + remoting/ios/session/remoting_client.h | 7 ++- remoting/ios/session/remoting_client.mm | 25 ++++------ 8 files changed, 86 insertions(+), 45 deletions(-) diff --git a/remoting/client/gesture_interpreter.cc b/remoting/client/gesture_interpreter.cc index b3d6dfa0efe8a1..2d5498ae6f3a78 100644 --- a/remoting/client/gesture_interpreter.cc +++ b/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); + } } } diff --git a/remoting/client/gesture_interpreter.h b/remoting/client/gesture_interpreter.h index d281b186cb90cb..fdbc57e3e5e9ba 100644 --- a/remoting/client/gesture_interpreter.h +++ b/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 input_strategy_; DesktopViewport viewport_; - RendererProxy* renderer_; - ChromotingSession* input_stub_; + RendererProxy* renderer_ = nullptr; + ChromotingSession* input_stub_ = nullptr; TouchInputStrategy::Gesture gesture_in_progress_; FlingAnimation pan_animation_; diff --git a/remoting/client/input/keyboard_interpreter.cc b/remoting/client/input/keyboard_interpreter.cc index 1a9194ca844e38..30abfe639b4225 100644 --- a/remoting/client/input/keyboard_interpreter.cc +++ b/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(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 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 keys; // Key press. @@ -51,6 +70,10 @@ void KeyboardInterpreter::HandleCtrlAltDeleteEvent() { } void KeyboardInterpreter::HandlePrintScreenEvent() { + if (!input_strategy_) { + return; + } + base::queue keys; // Key press. diff --git a/remoting/client/input/keyboard_interpreter.h b/remoting/client/input/keyboard_interpreter.h index 300b43adc82fb9..6f6d7e3cbe30b4 100644 --- a/remoting/client/input/keyboard_interpreter.h +++ b/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. diff --git a/remoting/client/ui/desktop_viewport.cc b/remoting/client/ui/desktop_viewport.cc index 6abbfb8a94aed7..7f6de72b3c17f9 100644 --- a/remoting/client/ui/desktop_viewport.cc +++ b/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_); } } diff --git a/remoting/ios/app/host_view_controller.mm b/remoting/ios/app/host_view_controller.mm index 905e662fc7e5d9..7f0e59ca881989 100644 --- a/remoting/ios/app/host_view_controller.mm +++ b/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 diff --git a/remoting/ios/session/remoting_client.h b/remoting/ios/session/remoting_client.h index 3c8d1824e44abc..85e732901f26d1 100644 --- a/remoting/ios/session/remoting_client.h +++ b/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; diff --git a/remoting/ios/session/remoting_client.mm b/remoting/ios/session/remoting_client.mm index 41ba4dc4207bb2..a8c7453b2fd259 100644 --- a/remoting/ios/session/remoting_client.mm +++ b/remoting/ios/session/remoting_client.mm @@ -65,9 +65,9 @@ @interface RemotingClient () { std::unique_ptr _sessonDelegate; ClientSessionDetails* _sessionDetails; remoting::protocol::SecretFetchedCallback _secretFetchedCallback; + remoting::GestureInterpreter _gestureInterpreter; + remoting::KeyboardInterpreter _keyboardInterpreter; std::unique_ptr _renderer; - std::unique_ptr _gestureInterpreter; - std::unique_ptr _keyboardInterpreter; std::unique_ptr _audioPlayer; std::unique_ptr _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