Reland: Increase reporting of accessibility errors (Round 2)

Make [de]serialization errors fatal for any kind of build used to
debug Chromium errors. This also restores the fail fast behavior in
ADDRESS_SANITIZER, which was removed in CL:2468238, because it
was mistakenly believed the address sanitizer builds had DCHECks on.

TBR=dmazzoni@chromium.org,sadrul@chromium.org

AX-RelNotes: n/a
Bug: None
Change-Id: I3a3651bf8732071cfb25f25eb4fc88ffa6a13855
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2486119Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818776}

content/browser/renderer_host/render_frame_host_impl.cc

@@ -239,6 +239,7 @@
 #include "third_party/blink/public/mojom/timing/resource_timing.mojom.h"
 #include "third_party/blink/public/mojom/usb/web_usb_service.mojom.h"
 #include "third_party/blink/public/mojom/webauthn/virtual_authenticator.mojom.h"
+#include "ui/accessibility/ax_common.h"
 #include "ui/accessibility/ax_tree.h"
 #include "ui/accessibility/ax_tree_id_registry.h"
 #include "ui/accessibility/ax_tree_update.h"
@@ -267,14 +268,14 @@ using base::TimeDelta;
 
 namespace content {
 
-#if DCHECK_IS_ON() || !defined(NDEBUG)
-// Wrapping this in DCHECK_IS_ON() will enable it to run on
-// clusterfuzz, which should help narrow down illegal ax trees more quickly.
+#if defined(AX_FAIL_FAST_BUILD)
+// Enable fast fails on clusterfuzz and other builds used to debug Chrome,
+// which should help narrow down illegal ax trees more quickly.
 // static
 int RenderFrameHostImpl::max_accessibility_resets_ = 0;
 #else
 int RenderFrameHostImpl::max_accessibility_resets_ = 4;
-#endif
+#endif  // AX_FAIL_FAST_BUILD
 
 struct RenderFrameHostOrProxy {
   RenderFrameHostImpl* const frame;

ui/accessibility/ax_common.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_ACCESSIBILITY_AX_COMMON_H_
+#define UI_ACCESSIBILITY_AX_COMMON_H_
+
+#if (!defined(NDEBUG) || defined(ADDRESS_SANITIZER) ||            \
+     defined(LEAK_SANITIZER) || defined(MEMORY_SANITIZER) ||      \
+     defined(THREAD_SANITIZER) || defined(UNDEFINED_SANITIZER) || \
+     DCHECK_IS_ON()) &&                                           \
+    !defined(OS_IOS)
+// Enable fast fails on clusterfuzz and other builds used to debug Chrome,
+// in order to help narrow down illegal states more quickly.
+#define AX_FAIL_FAST_BUILD
+#endif
+
+#endif  // UI_ACCESSIBILITY_AX_COMMON_H_

ui/accessibility/ax_tree_serializer.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "ui/accessibility/ax_common.h"
 #include "ui/accessibility/ax_export.h"
 #include "ui/accessibility/ax_tree_source.h"
 #include "ui/accessibility/ax_tree_update.h"
@@ -392,11 +393,11 @@ ClientTreeNode*
 AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::GetClientTreeNodeParent(
     ClientTreeNode* obj) {
   ClientTreeNode* parent = obj->parent;
-#if DCHECK_IS_ON() || !defined(NDEBUG)
+#if defined(AX_FAIL_FAST_BUILD)
   if (!parent)
     return nullptr;
   DCHECK(ClientTreeNodeById(parent->id)) << "Parent not in id map.";
-#endif  // DCHECK_IS_ON() || !defined(NDEBUG)
+#endif  // defined(AX_FAIL_FAST_BUILD)
   return parent;
 }
 
@@ -579,7 +580,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
 
     ClientTreeNode* client_child = ClientTreeNodeById(new_child_id);
     if (client_child && GetClientTreeNodeParent(client_child) != client_node) {
-#if DCHECK_IS_ON() || !defined(NDEBUG)
+#if defined(AX_FAIL_FAST_BUILD)
       // TODO(accessibility) Remove all cases where this occurs and re-add
       // NOTREACHED(), or add a histogram.
       // This condition leads to performance problems. It will
@@ -587,7 +588,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
       NOTREACHED()
 #else
       LOG(ERROR)
-#endif
+#endif  // defined(AX_FAIL_FAST_BUILD)
           << "Illegal reparenting detected: "
           << "\nPassed-in parent: "
           << tree_->GetDebugString(tree_->GetFromId(client_node->id))
@@ -673,7 +674,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
       new_child->invalid = false;
       client_node->children.push_back(new_child);
       if (ClientTreeNodeById(child_id)) {
-#if DCHECK_IS_ON() || !defined(NDEBUG)
+#if defined(AX_FAIL_FAST_BUILD)
         // TODO(accessibility) Remove all cases where this occurs and re-add
         // NOTREACHED(), or add a histogram.
         // This condition leads to performance problems. It will
@@ -681,7 +682,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
         NOTREACHED()
 #else
         LOG(ERROR)
-#endif
+#endif  // defined(AX_FAIL_FAST_BUILD)
             << "Child id " << child_id << " already exists in map."
             << "\nChild is "
             << tree_->GetDebugString(tree_->GetFromId(child_id))

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -13,9 +13,9 @@
 #include "base/strings/string_number_conversions.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/accessibility/ax_common.h"
 #include "ui/accessibility/ax_node.h"
 #include "ui/accessibility/ax_serializable_tree.h"
-#include "ui/accessibility/ax_tree.h"
 
 using testing::UnorderedElementsAre;
 
@@ -342,11 +342,9 @@ TEST_F(AXTreeSerializerTest, MaximumSerializedNodeCount) {
   ASSERT_EQ(5u, update.nodes.size());
 }
 
-#if !defined(ADDRESS_SANITIZER) && defined(NDEBUG) && !DCHECK_IS_ON()
+#if !defined(AX_FAIL_FAST_BUILD)
 // If duplicate ids are encountered, it returns an error and the next
 // update will re-send the entire tree.
-// Test does not work with address sanitizer -- if EXPECT_DEATH is used to
-// catch the "Illegal parenting" NOTREACHED(), an ASAN crash is still generated.
 TEST_F(AXTreeSerializerTest, DuplicateIdsReturnsErrorAndFlushes) {
   // (1 (2 (3 (4) 5)))
   treedata0_.root_id = 1;
@@ -397,7 +395,7 @@ TEST_F(AXTreeSerializerTest, DuplicateIdsReturnsErrorAndFlushes) {
   serializer_->SerializeChanges(tree1_->GetFromId(7), &update);
   ASSERT_EQ(5u, update.nodes.size());
 }
-#endif
+#endif  // !defined(AX_FAIL_FAST_BUILD)
 
 // If a tree serializer is reset, that means it doesn't know about
 // the state of the client tree anymore. The safest thing to do in