Update destruction tracking to safer pattern

Chrome sometimes needs to detect when an object has been deleted in
the middle of a member function. Manually maintaining a destroyed
pointer/flag is error prone and verbose so this change switches to using
WeakPtr.

In the NativeMenuWin case the destroyed pointer wasn't even used, making
deleting it particularly easy.

Idea from dcheng@

Bug: 1152152
Change-Id: Ie913fa6255c5db4667e59b724732df2dbedf5580
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637268Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845538}

ui/events/scoped_target_handler.cc

@@ -12,31 +12,23 @@ namespace ui {
 
 ScopedTargetHandler::ScopedTargetHandler(EventTarget* target,
                                          EventHandler* handler)
-    : destroyed_flag_(nullptr), target_(target), new_handler_(handler) {
+    : target_(target), new_handler_(handler) {
   original_handler_ = target_->SetTargetHandler(this);
 }
 
 ScopedTargetHandler::~ScopedTargetHandler() {
   EventHandler* handler = target_->SetTargetHandler(original_handler_);
   DCHECK_EQ(this, handler);
-  if (destroyed_flag_)
-    *destroyed_flag_ = true;
 }
 
 void ScopedTargetHandler::OnEvent(Event* event) {
   if (original_handler_) {
-    bool destroyed = false;
-    bool* old_destroyed_flag = destroyed_flag_;
-    destroyed_flag_ = &destroyed;
+    auto weak_this = weak_factory_.GetWeakPtr();
 
     original_handler_->OnEvent(event);
 
-    if (destroyed) {
-      if (old_destroyed_flag)
-        *old_destroyed_flag = true;
+    if (!weak_this)
       return;
-    }
-    destroyed_flag_ = old_destroyed_flag;
   }
 
   // This check is needed due to nested event loops when starting DragDrop.

ui/events/scoped_target_handler.h

@@ -6,6 +6,7 @@
 #define UI_EVENTS_SCOPED_TARGET_HANDLER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string_piece.h"
 #include "ui/events/event_handler.h"
 #include "ui/events/events_export.h"
@@ -29,9 +30,6 @@ class EVENTS_EXPORT ScopedTargetHandler : public EventHandler {
   base::StringPiece GetLogContext() const override;
 
  private:
-  // If non-null the destructor sets this to true. This is set while handling
-  // an event and used to detect if |this| has been deleted.
-  bool* destroyed_flag_;
 
   // An EventTarget that has its target handler replaced with |this| for a life
   // time of |this|.
@@ -43,6 +41,10 @@ class EVENTS_EXPORT ScopedTargetHandler : public EventHandler {
   // A new handler that gets events in addition to the |original_handler_|.
   EventHandler* new_handler_;
 
+  // Used to detect if handling an event has caused |this| to be deleted. Must
+  // be last.
+  base::WeakPtrFactory<ScopedTargetHandler> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ScopedTargetHandler);
 };
 

ui/views/controls/menu/native_menu_win.cc

@@ -51,12 +51,9 @@ NativeMenuWin::NativeMenuWin(ui::MenuModel* model, HWND system_menu_for)
                   !system_menu_for),
       system_menu_for_(system_menu_for),
       first_item_index_(0),
-      parent_(nullptr),
-      destroyed_flag_(nullptr) {}
+      parent_(nullptr) {}
 
 NativeMenuWin::~NativeMenuWin() {
-  if (destroyed_flag_)
-    *destroyed_flag_ = true;
   items_.clear();
   DestroyMenu(menu_);
 }

ui/views/controls/menu/native_menu_win.h

@@ -100,11 +100,6 @@ class VIEWS_EXPORT NativeMenuWin {
   // If we're a submenu, this is our parent.
   NativeMenuWin* parent_;
 
-  // If non-null the destructor sets this to true. This is set to non-null while
-  // the menu is showing. It is used to detect if the menu was deleted while
-  // running.
-  bool* destroyed_flag_;
-
   DISALLOW_COPY_AND_ASSIGN(NativeMenuWin);
 };