MacPWAs: Trust content::NativeWebKeyboardEvent serialization to be wrong

A number of bugs have come out of the fact that
content::NativeWebKeyboardEvent appears to provide a mechanism to pass
an NSEvent across processes via
    NSEvent -> content::NativeWebKeyboardEvent -> (mojo) ->
    content::NativeWebKeyboardEvent -> NSEvent
In practice, the NSEvent reconstructed after the
content::NativeWebKeyboardEvent was passed over mojo is not even
remotely accurate.

Add helper functions to serialize NSEvents using the OS-provided
CGEventCreateData and CGEventCreateFromData functions. When passing a
content::NativeWebKeyboardEvent over mojo, pass the serialization
alongside it, and replace the result's os_event.

Use this approach instead of updating the mojo serialization for
content::NativeWebKeyboardEvent because we don't want to be calling
CGEventCreateFromData on data sent from the renderer process.

Remove the accumulated workarounds for these issues.

R=dominickn
TBR=avi (content/ OWNERship)

Bug: 919167, 943197, 964052, 942690
Change-Id: Ie7321229daf34629ddc9037ab733b40828bc62c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1659662
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669972}

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -64,16 +64,12 @@
   // TODO(erikchen): Detect symbolic hot keys, and force control to be passed
   // back to AppKit so that it can handle it correctly.
   // https://crbug.com/846893.
-  auto* bridge =
-      remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window);
 
   NSResponder* responder = [window firstResponder];
   if ([responder conformsToProtocol:@protocol(CommandDispatcherTarget)]) {
     NSObject<CommandDispatcherTarget>* target =
         static_cast<NSObject<CommandDispatcherTarget>*>(responder);
     if ([target isKeyLocked:event]) {
-      if (bridge)
-        bridge->SaveKeyEventForRedispatch(event);
       return ui::PerformKeyEquivalentResult::kUnhandled;
     }
   }
@@ -99,6 +95,8 @@
   // highlighting of the NSMenu.
   CommandForKeyEventResult result = CommandForKeyEvent(event);
   if (result.found()) {
+    auto* bridge =
+        remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window);
     if (bridge) {
       bool was_executed = false;
       bridge->host()->ExecuteCommand(
@@ -109,8 +107,6 @@
     }
   }
 
-  if (bridge)
-    bridge->SaveKeyEventForRedispatch(event);
   return ui::PerformKeyEquivalentResult::kUnhandled;
 }
 

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.h

@@ -185,10 +185,6 @@ class REMOTE_COCOA_APP_SHIM_EXPORT NativeWidgetNSWindowBridge
   // Redispatch a keyboard event using the widget's window's CommandDispatcher.
   // Return true if the event is handled.
   bool RedispatchKeyEvent(NSEvent* event);
-  // Save an NSEvent to be used at the mojo version of RedispatchKeyEvent,
-  // rather than (inaccurately) reconstructing the NSEvent.
-  // https://crbug.com/942690
-  void SaveKeyEventForRedispatch(NSEvent* event);
 
   // display::DisplayObserver:
   void OnDisplayMetricsChanged(const display::Display& display,
@@ -241,12 +237,8 @@ class REMOTE_COCOA_APP_SHIM_EXPORT NativeWidgetNSWindowBridge
   void UpdateTooltip() override;
   void AcquireCapture() override;
   void ReleaseCapture() override;
-  void RedispatchKeyEvent(uint64_t type,
-                          uint64_t modifier_flags,
-                          double timestamp,
-                          const base::string16& characters,
-                          const base::string16& characters_ignoring_modifiers,
-                          uint32_t key_code) override;
+  void RedispatchKeyEvent(
+      const std::vector<uint8_t>& native_event_data) override;
 
   // Return true if [NSApp updateWindows] needs to be called after updating the
   // TextInputClient.
@@ -302,7 +294,6 @@ class REMOTE_COCOA_APP_SHIM_EXPORT NativeWidgetNSWindowBridge
   base::scoped_nsobject<ViewsNSWindowDelegate> window_delegate_;
   base::scoped_nsobject<NSObject<CommandDispatcherDelegate>>
       window_command_dispatcher_delegate_;
-  base::scoped_nsobject<NSEvent> saved_redispatch_event_;
 
   base::scoped_nsobject<BridgedContentView> bridged_view_;
   std::unique_ptr<remote_cocoa::ScopedNSViewIdMapping> bridged_view_id_mapping_;

components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm

@@ -39,6 +39,7 @@
 #include "ui/base/layout.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/display/screen.h"
+#include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/gfx/geometry/dip_util.h"
 #import "ui/gfx/mac/coordinate_conversion.h"
 #import "ui/gfx/mac/nswindow_frame_controls.h"
@@ -1096,10 +1097,6 @@ bool NativeWidgetNSWindowBridge::RedispatchKeyEvent(NSEvent* event) {
   return [[window_ commandDispatcher] redispatchKeyEvent:event];
 }
 
-void NativeWidgetNSWindowBridge::SaveKeyEventForRedispatch(NSEvent* event) {
-  saved_redispatch_event_.reset([event retain]);
-}
-
 NSWindow* NativeWidgetNSWindowBridge::ns_window() {
   return window_.get();
 }
@@ -1251,42 +1248,8 @@ bool NativeWidgetNSWindowBridge::NeedsUpdateWindows() {
 }
 
 void NativeWidgetNSWindowBridge::RedispatchKeyEvent(
-    uint64_t type,
-    uint64_t modifier_flags,
-    double timestamp,
-    const base::string16& characters,
-    const base::string16& characters_ignoring_modifiers,
-    uint32_t key_code) {
-  // If we saved an event for redispatch, and that event looks similar to the
-  // (potentially mangled) event parameters that we received, then use the saved
-  // event.
-  // https://crbug.com/942690
-  if (saved_redispatch_event_) {
-    // Consider two events to have the same timestamp if they are within 0.1 ms.
-    constexpr double kTimestampThreshold = 0.0001;
-    if ([saved_redispatch_event_ type] == type &&
-        base::SysNSStringToUTF16([saved_redispatch_event_ characters]) ==
-            characters &&
-        std::fabs([saved_redispatch_event_ timestamp] - timestamp) <
-            kTimestampThreshold) {
-      RedispatchKeyEvent(saved_redispatch_event_.autorelease());
-      return;
-    }
-    saved_redispatch_event_.reset();
-  }
-  NSEvent* event =
-      [NSEvent keyEventWithType:static_cast<NSEventType>(type)
-                             location:NSZeroPoint
-                        modifierFlags:modifier_flags
-                            timestamp:timestamp
-                         windowNumber:[window_ windowNumber]
-                              context:nil
-                           characters:base::SysUTF16ToNSString(characters)
-          charactersIgnoringModifiers:base::SysUTF16ToNSString(
-                                          characters_ignoring_modifiers)
-                            isARepeat:NO
-                              keyCode:key_code];
-  RedispatchKeyEvent(event);
+    const std::vector<uint8_t>& native_event_data) {
+  RedispatchKeyEvent(ui::EventFromData(native_event_data));
 }
 
 ////////////////////////////////////////////////////////////////////////////////

components/remote_cocoa/common/native_widget_ns_window.mojom

@@ -204,15 +204,5 @@ interface NativeWidgetNSWindow {
   ReleaseCapture();
 
   // Redispatch a keyboard event using the widget's window's CommandDispatcher.
-  // Note that the callers of this method use content::NativeWebKeyboardEvent,
-  // which would  introduce a layering violation here. Specify explicitly as
-  // arguments only what is used in constructing the |os_event| member of
-  // content::NativeWebKeyboardEvent, to avoid this layering violation.
-  RedispatchKeyEvent(
-      uint64 type,
-      uint64 modifier_flags,
-      double timestamp,
-      mojo_base.mojom.String16 characters,
-      mojo_base.mojom.String16 characters_ignoring_modifiers,
-      uint32 key_code);
+  RedispatchKeyEvent(array<uint8> native_event_data);
 };

content/app_shim_remote_cocoa/ns_view_bridge_factory_impl.mm

@@ -20,6 +20,7 @@
 #include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "ui/accelerated_widget_mac/window_resize_helper_mac.h"
 #include "ui/base/cocoa/remote_accessibility_api.h"
+#include "ui/events/cocoa/cocoa_event_utils.h"
 
 namespace remote_cocoa {
 
@@ -66,12 +67,8 @@ class RenderWidgetHostNSViewBridgeOwner
 
   void ForwardKeyboardEvent(const content::NativeWebKeyboardEvent& key_event,
                             const ui::LatencyInfo& latency_info) override {
-    const blink::WebKeyboardEvent* web_event =
-        static_cast<const blink::WebKeyboardEvent*>(&key_event);
-    std::unique_ptr<content::InputEvent> input_event =
-        std::make_unique<content::InputEvent>(*web_event, latency_info);
-    host_->ForwardKeyboardEvent(std::move(input_event),
-                                key_event.skip_in_browser);
+    std::vector<content::EditCommand> commands;
+    ForwardKeyboardEventWithCommands(key_event, latency_info, commands);
   }
   void ForwardKeyboardEventWithCommands(
       const content::NativeWebKeyboardEvent& key_event,
@@ -81,8 +78,11 @@ class RenderWidgetHostNSViewBridgeOwner
         static_cast<const blink::WebKeyboardEvent*>(&key_event);
     std::unique_ptr<content::InputEvent> input_event =
         std::make_unique<content::InputEvent>(*web_event, latency_info);
+    std::vector<uint8_t> native_event_data =
+        ui::EventToData(key_event.os_event);
     host_->ForwardKeyboardEventWithCommands(
-        std::move(input_event), key_event.skip_in_browser, commands);
+        std::move(input_event), native_event_data, key_event.skip_in_browser,
+        commands);
   }
   void RouteOrProcessMouseEvent(
       const blink::WebMouseEvent& web_event) override {

content/browser/renderer_host/render_widget_host_view_mac.h

@@ -340,11 +340,9 @@ class CONTENT_EXPORT RenderWidgetHostViewMac
   void OnDisplayChanged(const display::Display& display) override;
   void BeginKeyboardEvent() override;
   void EndKeyboardEvent() override;
-
-  void ForwardKeyboardEvent(std::unique_ptr<InputEvent> event,
-                            bool skip_in_browser) override;
   void ForwardKeyboardEventWithCommands(
       std::unique_ptr<InputEvent> event,
+      const std::vector<uint8_t>& native_event_data,
       bool skip_in_browser,
       const std::vector<EditCommand>& commands) override;
   void RouteOrProcessMouseEvent(std::unique_ptr<InputEvent> event) override;

content/browser/renderer_host/render_widget_host_view_mac.mm

@@ -57,6 +57,7 @@
 #include "ui/base/ui_base_features.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
+#include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/events/gesture_detection/gesture_provider_config_helper.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 #include "ui/events/keycodes/dom/dom_keyboard_layout_map.h"
@@ -1927,24 +1928,9 @@ void RenderWidgetHostViewMac::SetRemoteAccessibilityWindowToken(
 // mojom::RenderWidgetHostNSViewHost functions that translate events and
 // forward them to the RenderWidgetHostNSViewHostHelper implementation:
 
-void RenderWidgetHostViewMac::ForwardKeyboardEvent(
-    std::unique_ptr<InputEvent> input_event,
-    bool skip_in_browser) {
-  if (!input_event || !input_event->web_event ||
-      !blink::WebInputEvent::IsKeyboardEventType(
-          input_event->web_event->GetType())) {
-    DLOG(ERROR) << "Absent or non-KeyboardEventType event.";
-    return;
-  }
-  const blink::WebKeyboardEvent& keyboard_event =
-      static_cast<const blink::WebKeyboardEvent&>(*input_event->web_event);
-  NativeWebKeyboardEvent native_event(keyboard_event, nil);
-  native_event.skip_in_browser = skip_in_browser;
-  ForwardKeyboardEvent(native_event, input_event->latency_info);
-}
-
 void RenderWidgetHostViewMac::ForwardKeyboardEventWithCommands(
     std::unique_ptr<InputEvent> input_event,
+    const std::vector<uint8_t>& native_event_data,
     bool skip_in_browser,
     const std::vector<EditCommand>& commands) {
   if (!input_event || !input_event->web_event ||
@@ -1957,6 +1943,12 @@ void RenderWidgetHostViewMac::ForwardKeyboardEventWithCommands(
       static_cast<const blink::WebKeyboardEvent&>(*input_event->web_event);
   NativeWebKeyboardEvent native_event(keyboard_event, nil);
   native_event.skip_in_browser = skip_in_browser;
+  // The NSEvent constructed from the InputEvent sent over mojo is not even
+  // close to the original NSEvent, resulting in all sorts of bugs. Use the
+  // native event serialization to reconstruct the NSEvent.
+  // https://crbug.com/919167,943197,964052
+  [native_event.os_event release];
+  native_event.os_event = [ui::EventFromData(native_event_data) retain];
   ForwardKeyboardEventWithCommands(native_event, input_event->latency_info,
                                    commands);
 }

content/common/render_widget_host_ns_view.mojom

@@ -136,9 +136,9 @@ interface RenderWidgetHostNSViewHost {
 
   // Forward a keyboard event to the RenderWidgetHost that is currently handling
   // the key-down event.
-  ForwardKeyboardEvent(content.mojom.Event event, bool skip_in_browser);
   ForwardKeyboardEventWithCommands(
       content.mojom.Event event,
+      array<uint8> native_event_data,
       bool skip_in_browser,
       array<content.mojom.EditCommand> commands);
 

ui/events/cocoa/cocoa_event_utils.h

@@ -7,6 +7,8 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include <vector>
+
 #include "ui/events/events_export.h"
 
 namespace ui {
@@ -29,6 +31,13 @@ EVENTS_EXPORT int EventFlagsFromNSEventWithModifiers(NSEvent* event,
 // released.
 EVENTS_EXPORT bool IsKeyUpEvent(NSEvent* event);
 
+// Convert an NSEvent to an opaque serialization using CGEventCreateData.
+EVENTS_EXPORT std::vector<uint8_t> EventToData(NSEvent* event);
+
+// Create an NSEvent from an opaque serialization using CGEventCreateFromData.
+// The result is autoreleased.
+EVENTS_EXPORT NSEvent* EventFromData(const std::vector<uint8_t>& data);
+
 }  // namespace ui
 
 #endif  // UI_EVENTS_COCOA_COCOA_EVENT_UTILS_H_

ui/events/cocoa/cocoa_event_utils.mm

@@ -4,6 +4,7 @@
 
 #import "ui/events/cocoa/cocoa_event_utils.h"
 
+#include "base/mac/scoped_cftyperef.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/event_utils.h"
 
@@ -133,4 +134,20 @@ bool IsKeyUpEvent(NSEvent* event) {
   return false;
 }
 
+std::vector<uint8_t> EventToData(NSEvent* event) {
+  base::ScopedCFTypeRef<CFDataRef> cf_data(
+      CGEventCreateData(nullptr, [event CGEvent]));
+  const uint8_t* cf_data_ptr = CFDataGetBytePtr(cf_data.get());
+  size_t cf_data_size = CFDataGetLength(cf_data.get());
+  return std::vector<uint8_t>(cf_data_ptr, cf_data_ptr + cf_data_size);
+}
+
+NSEvent* EventFromData(const std::vector<uint8_t>& data) {
+  base::ScopedCFTypeRef<CFDataRef> cf_data(
+      CFDataCreate(nullptr, data.data(), data.size()));
+  base::ScopedCFTypeRef<CGEventRef> cg_event(
+      CGEventCreateFromData(nullptr, cf_data.get()));
+  return [NSEvent eventWithCGEvent:cg_event.get()];
+}
+
 }  // namespace ui

ui/views/cocoa/native_widget_mac_ns_window_host.mm

@@ -23,6 +23,7 @@
 #include "ui/base/ime/input_method.h"
 #include "ui/compositor/recyclable_compositor_mac.h"
 #include "ui/display/screen.h"
+#include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/mac/coordinate_conversion.h"
 #include "ui/native_theme/native_theme_mac.h"
@@ -646,11 +647,7 @@ bool NativeWidgetMacNSWindowHost::RedispatchKeyEvent(NSEvent* event) {
 
   // If the target window is out of process then always report the event as
   // handled (because it should never be handled in this process).
-  GetNSWindowMojo()->RedispatchKeyEvent(
-      [event type], [event modifierFlags], [event timestamp],
-      base::SysNSStringToUTF16([event characters]),
-      base::SysNSStringToUTF16([event charactersIgnoringModifiers]),
-      [event keyCode]);
+  GetNSWindowMojo()->RedispatchKeyEvent(ui::EventToData(event));
   return true;
 }