Actually serialize AXPosition when passing to AXTextMarker.

Many Mac accessibility API use an AXTextMarker, which is an
opaque application-specific struct representing a position in
a document. Chrome implements these using AXPosition
(in particular a subclass BrowserAccessibilityPosition).

The code used the word "serialized" but it was actually
copying the raw bytes from a BrowserAccessibilityPosition
and then constructing a new C++ object with these bytes
later, which isn't defined behavior and may be causing
problems on macOS 10.15.

Instead, implement actual Serialize and Unserialize methods
on AXPosition and make use of those instead.

Tested with a unit test of AXPosition (based on the existing
test of Clone) and by manually manipulating some editable
text in Chrome with VoiceOver running, to ensure the code is
working correctly.

Bug: 998896
Change-Id: Ifb64b19ebdc96a8d36522456c77bca23cb58d552
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776901
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691970}

content/browser/accessibility/browser_accessibility_cocoa.mm

@@ -38,6 +38,8 @@
 
 using BrowserAccessibilityPositionInstance =
     content::BrowserAccessibilityPosition::AXPositionInstance;
+using SerializedPosition =
+    content::BrowserAccessibilityPosition::SerializedPosition;
 using AXPlatformRange =
     ui::AXRange<BrowserAccessibilityPositionInstance::element_type>;
 using AXTextMarkerRangeRef = CFTypeRef;
@@ -54,6 +56,10 @@ using content::OneShotAccessibilityTreeSearch;
 using ui::AXNodeData;
 using ui::AXTreeIDRegistry;
 
+static_assert(
+    std::is_trivially_copyable<SerializedPosition>::value,
+    "SerializedPosition must be POD because it's used to back an AXTextMarker");
+
 namespace {
 
 // Private WebKit accessibility attributes.
@@ -250,21 +256,24 @@ AXTextMarkerRef AXTextMarkerRangeCopyEndMarker(
 
 // AXTextMarkerCreate copies from data buffer given to it.
 id CreateTextMarker(BrowserAccessibilityPositionInstance position) {
+  SerializedPosition serialized = position->Serialize();
   AXTextMarkerRef text_marker = AXTextMarkerCreate(
-      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(position.get()),
-      sizeof(BrowserAccessibilityPosition));
+      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(&serialized),
+      sizeof(SerializedPosition));
   return [static_cast<id>(text_marker) autorelease];
 }
 
 // |range| is destructed at the end of this method. |anchor| and |focus| are
 // copied into the individual text markers.
 id CreateTextMarkerRange(const AXPlatformRange range) {
+  SerializedPosition serialized_anchor = range.anchor()->Serialize();
+  SerializedPosition serialized_focus = range.focus()->Serialize();
   base::ScopedCFTypeRef<AXTextMarkerRef> start_marker(AXTextMarkerCreate(
-      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(range.anchor()),
-      sizeof(BrowserAccessibilityPosition)));
+      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(&serialized_anchor),
+      sizeof(SerializedPosition)));
   base::ScopedCFTypeRef<AXTextMarkerRef> end_marker(AXTextMarkerCreate(
-      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(range.focus()),
-      sizeof(BrowserAccessibilityPosition)));
+      kCFAllocatorDefault, reinterpret_cast<const UInt8*>(&serialized_focus),
+      sizeof(SerializedPosition)));
   AXTextMarkerRangeRef marker_range =
       AXTextMarkerRangeCreate(kCFAllocatorDefault, start_marker, end_marker);
   return [static_cast<id>(marker_range) autorelease];
@@ -273,22 +282,15 @@ id CreateTextMarkerRange(const AXPlatformRange range) {
 BrowserAccessibilityPositionInstance CreatePositionFromTextMarker(
     AXTextMarkerRef text_marker) {
   DCHECK(text_marker);
-  if (AXTextMarkerGetLength(text_marker) !=
-      sizeof(BrowserAccessibilityPosition))
+  if (AXTextMarkerGetLength(text_marker) != sizeof(SerializedPosition))
     return BrowserAccessibilityPosition::CreateNullPosition();
+
   const UInt8* source_buffer = AXTextMarkerGetBytePtr(text_marker);
   if (!source_buffer)
     return BrowserAccessibilityPosition::CreateNullPosition();
-  UInt8* destination_buffer = new UInt8[sizeof(BrowserAccessibilityPosition)];
-  std::memcpy(destination_buffer, source_buffer,
-              sizeof(BrowserAccessibilityPosition));
-  BrowserAccessibilityPosition::AXPositionInstance position(
-      reinterpret_cast<
-          BrowserAccessibilityPosition::AXPositionInstance::pointer>(
-          destination_buffer));
-  if (!position)
-    return BrowserAccessibilityPosition::CreateNullPosition();
-  return position;
+
+  return BrowserAccessibilityPosition::Unserialize(
+      *reinterpret_cast<const SerializedPosition*>(source_buffer));
 }
 
 AXPlatformRange CreateRangeFromTextMarkerRange(

ui/accessibility/ax_node_position_unittest.cc

@@ -478,6 +478,60 @@ TEST_F(AXPositionTest, Clone) {
   EXPECT_EQ(AXNodePosition::INVALID_INDEX, copy_position->child_index());
 }
 
+TEST_F(AXPositionTest, Serialize) {
+  TestPositionType null_position = AXNodePosition::CreateNullPosition();
+  ASSERT_NE(nullptr, null_position);
+  TestPositionType copy_position =
+      AXNodePosition::Unserialize(null_position->Serialize());
+  ASSERT_NE(nullptr, copy_position);
+  EXPECT_TRUE(copy_position->IsNullPosition());
+
+  TestPositionType tree_position = AXNodePosition::CreateTreePosition(
+      tree_.data().tree_id, root_.id, 1 /* child_index */);
+  ASSERT_NE(nullptr, tree_position);
+  copy_position = AXNodePosition::Unserialize(tree_position->Serialize());
+  ASSERT_NE(nullptr, copy_position);
+  EXPECT_TRUE(copy_position->IsTreePosition());
+  EXPECT_EQ(root_.id, copy_position->anchor_id());
+  EXPECT_EQ(1, copy_position->child_index());
+  EXPECT_EQ(AXNodePosition::INVALID_OFFSET, copy_position->text_offset());
+
+  tree_position = AXNodePosition::CreateTreePosition(
+      tree_.data().tree_id, root_.id, AXNodePosition::BEFORE_TEXT);
+  ASSERT_NE(nullptr, tree_position);
+  copy_position = AXNodePosition::Unserialize(tree_position->Serialize());
+  ASSERT_NE(nullptr, copy_position);
+  EXPECT_TRUE(copy_position->IsTreePosition());
+  EXPECT_EQ(root_.id, copy_position->anchor_id());
+  EXPECT_EQ(AXNodePosition::BEFORE_TEXT, copy_position->child_index());
+  EXPECT_EQ(AXNodePosition::INVALID_OFFSET, copy_position->text_offset());
+
+  TestPositionType text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, text_field_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kUpstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  copy_position = AXNodePosition::Unserialize(text_position->Serialize());
+  ASSERT_NE(nullptr, copy_position);
+  EXPECT_TRUE(copy_position->IsTextPosition());
+  EXPECT_EQ(text_field_.id, copy_position->anchor_id());
+  EXPECT_EQ(0, copy_position->text_offset());
+  EXPECT_EQ(ax::mojom::TextAffinity::kUpstream, copy_position->affinity());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, text_field_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  copy_position = AXNodePosition::Unserialize(text_position->Serialize());
+  ASSERT_NE(nullptr, copy_position);
+  EXPECT_TRUE(copy_position->IsTextPosition());
+  EXPECT_EQ(text_field_.id, copy_position->anchor_id());
+  EXPECT_EQ(0, copy_position->text_offset());
+  EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, copy_position->affinity());
+  EXPECT_EQ(AXNodePosition::INVALID_INDEX, copy_position->child_index());
+}
+
 TEST_F(AXPositionTest, GetTextFromNullPosition) {
   TestPositionType text_position = AXNodePosition::CreateNullPosition();
   ASSERT_NE(nullptr, text_position);

ui/accessibility/ax_position.h

@@ -10,6 +10,7 @@
 #include <memory>
 #include <ostream>
 #include <string>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -160,6 +161,48 @@ class AXPosition {
 
   virtual AXPositionInstance Clone() const = 0;
 
+  // A serialization of a position as POD. Not for sharing on disk or sharing
+  // across thread or process boundaries, just for passing a position to an
+  // API that works with positions as opaque objects.
+  struct SerializedPosition {
+    AXPositionKind kind;
+    int32_t anchor_id;
+    int child_index;
+    int text_offset;
+    ax::mojom::TextAffinity affinity;
+    char tree_id[33];
+  };
+
+  static_assert(std::is_trivially_copyable<SerializedPosition>::value,
+                "SerializedPosition must be POD");
+
+  SerializedPosition Serialize() {
+    SerializedPosition result;
+    result.kind = kind_;
+
+    // A tree ID can be serialized as a 32-byte string.
+    std::string tree_id_string = tree_id_.ToString();
+    DCHECK_LE(tree_id_string.size(), 32U);
+    strncpy(result.tree_id, tree_id_string.c_str(), 32);
+    result.tree_id[32] = 0;
+
+    result.anchor_id = anchor_id_;
+    result.child_index = child_index_;
+    result.text_offset = text_offset_;
+    result.affinity = affinity_;
+    return result;
+  }
+
+  static AXPositionInstance Unserialize(
+      const SerializedPosition& serialization) {
+    AXPositionInstance new_position(new AXPositionType());
+    new_position->Initialize(serialization.kind,
+                             ui::AXTreeID::FromString(serialization.tree_id),
+                             serialization.anchor_id, serialization.child_index,
+                             serialization.text_offset, serialization.affinity);
+    return new_position;
+  }
+
   virtual bool IsIgnoredPosition() const { return false; }
 
   virtual AXPositionInstance AsUnignoredTextPosition(