Commit 3fff88ce authored by Bruce Dawson's avatar Bruce Dawson Committed by Chromium LUCI CQ

Revert "Track progress through View destructor"

This reverts commit 24e521c6.

Reason for revert: bug has been found and fixed

Original change's description:
> Track progress through View destructor
>
> The View destructor ends up in an impossible state which means that
> execution must have branched to the middle of it, but it is not clear
> how this could happen. This change adds an array of line numbers that is
> filled in as the destructor runs to leave us some bread crumbs. It also
> records the vtable of the about-to-be-deleted child object so that we
> can investigate if this is a type-related bug.
>
> Bug: 1152152
> Change-Id: I386507974a8b6ccd1914e0ec62151456411f0c19
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572793
> Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#833875}

TBR=robliao@chromium.org,brucedawson@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1152152
Change-Id: I62871ab84d1a219c9eed67bc2ce3d0b4ceed44c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580545Reviewed-by: default avatarBruce Dawson <brucedawson@chromium.org>
Reviewed-by: default avatarRobert Liao <robliao@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834999}
parent eafd059d
...@@ -11,7 +11,6 @@ ...@@ -11,7 +11,6 @@
#include "base/check_op.h" #include "base/check_op.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/containers/adapters.h" #include "base/containers/adapters.h"
#include "base/debug/alias.h"
#include "base/feature_list.h" #include "base/feature_list.h"
#include "base/i18n/rtl.h" #include "base/i18n/rtl.h"
#include "base/macros.h" #include "base/macros.h"
...@@ -192,19 +191,8 @@ View::View() { ...@@ -192,19 +191,8 @@ View::View() {
} }
View::~View() { View::~View() {
// Line number history to see the path through this function prior to a
// mysterious crash (https://crbug.com/1152152).
constexpr size_t kNumEntries = 16;
int line_number_trace[kNumEntries] = {};
base::debug::Alias(&line_number_trace[0]);
size_t next_line_entry = 0;
base::debug::Alias(&next_line_entry);
void* vtable = nullptr;
base::debug::Alias(&vtable);
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
if (parent_) if (parent_)
parent_->RemoveChildView(this); parent_->RemoveChildView(this);
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
// This view should have been removed from the focus list by now. // This view should have been removed from the focus list by now.
DCHECK_EQ(next_focusable_view_, nullptr); DCHECK_EQ(next_focusable_view_, nullptr);
...@@ -216,48 +204,34 @@ View::~View() { ...@@ -216,48 +204,34 @@ View::~View() {
// layout managers access child view properties, this would result in a // layout managers access child view properties, this would result in a
// use-after-free error. // use-after-free error.
layout_manager_.reset(); layout_manager_.reset();
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
{ {
internal::ScopedChildrenLock lock(this); internal::ScopedChildrenLock lock(this);
for (auto* child : children_) { for (auto* child : children_) {
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
child->parent_ = nullptr; child->parent_ = nullptr;
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
// Since all children are removed here, it is safe to set // Since all children are removed here, it is safe to set
// |child|'s focus list pointers to null and expect any references // |child|'s focus list pointers to null and expect any references
// to |child| will be removed subsequently. // to |child| will be removed subsequently.
child->next_focusable_view_ = nullptr; child->next_focusable_view_ = nullptr;
child->previous_focusable_view_ = nullptr; child->previous_focusable_view_ = nullptr;
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
if (!child->owned_by_client_) { if (!child->owned_by_client_)
// Extract the vtable pointer to allow post-mortem identification of the
// object type.
if (child)
vtable = reinterpret_cast<void**>(child)[0];
delete child; delete child;
}
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
} }
} }
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
for (ViewObserver& observer : observers_) for (ViewObserver& observer : observers_)
observer.OnViewIsDeleting(this); observer.OnViewIsDeleting(this);
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
for (ui::Layer* layer_beneath : layers_beneath_) for (ui::Layer* layer_beneath : layers_beneath_)
layer_beneath->RemoveObserver(this); layer_beneath->RemoveObserver(this);
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
// Clearing properties explicitly here lets us guarantee that properties // Clearing properties explicitly here lets us guarantee that properties
// outlive |this| (at least the View part of |this|). This is intentionally // outlive |this| (at least the View part of |this|). This is intentionally
// called at the end so observers can examine properties inside // called at the end so observers can examine properties inside
// OnViewIsDeleting(), for instance. // OnViewIsDeleting(), for instance.
ClearProperties(); ClearProperties();
line_number_trace[(next_line_entry++) % kNumEntries] = __LINE__;
} }
// Tree operations ------------------------------------------------------------- // Tree operations -------------------------------------------------------------
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment