Revert recent changes in SelectFileDialogImplGTK

The below changes were added to fix a crash on GTK4.  However, this introduced a
crash on GTK3.  I'm unable to reproduce the crash and I'm going to be afk for 4
weeks after today, so I'm reverting these changes since GTK3 is currently
supported but GTK4 is not, and I want to get the revert merged to M71.  The
changes can be relanded in the future once we have reliable repro steps.

GTK4: Fix event processing after closing dialogs
Fix crash in SelectFileDialogImplGTK::OnFileChooserDestroyThunk.
[GTK] Speculative fix for crash in SelectFileDialogImplGTK
Fix heap-use-after-free in SelectFileDialogImplGTK

BUG=880073,901282
R=sky

Change-Id: Id046e9a54527e4df251884e6447f8830a3b859d3
Reviewed-on: https://chromium-review.googlesource.com/c/1314636Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605021}

chrome/browser/ui/libgtkui/gtk_util.cc

@@ -31,6 +31,8 @@
 
 namespace {
 
+const char kAuraTransientParent[] = "aura-transient-parent";
+
 void CommonInitFromCommandLine(const base::CommandLine& command_line) {
 #if GTK_CHECK_VERSION(3, 90, 0)
   gtk_init();
@@ -136,6 +138,19 @@ void SetGtkTransientForAura(GtkWidget* dialog, aura::Window* parent) {
   XSetTransientForHint(GDK_WINDOW_XDISPLAY(gdk_window),
                        GDK_WINDOW_XID(gdk_window),
                        parent->GetHost()->GetAcceleratedWidget());
+
+  // We also set the |parent| as a property of |dialog|, so that we can unlink
+  // the two later.
+  g_object_set_data(G_OBJECT(dialog), kAuraTransientParent, parent);
+}
+
+aura::Window* GetAuraTransientParent(GtkWidget* dialog) {
+  return reinterpret_cast<aura::Window*>(
+      g_object_get_data(G_OBJECT(dialog), kAuraTransientParent));
+}
+
+void ClearAuraTransientParent(GtkWidget* dialog) {
+  g_object_set_data(G_OBJECT(dialog), kAuraTransientParent, nullptr);
 }
 
 void ParseButtonLayout(const std::string& button_string,

chrome/browser/ui/libgtkui/gtk_util.h

@@ -53,6 +53,12 @@ void TurnButtonBlue(GtkWidget* button);
 // it above |parent|. Do nothing if |parent| is nullptr.
 void SetGtkTransientForAura(GtkWidget* dialog, aura::Window* parent);
 
+// Gets the transient parent aura window for |dialog|.
+aura::Window* GetAuraTransientParent(GtkWidget* dialog);
+
+// Clears the transient parent for |dialog|.
+void ClearAuraTransientParent(GtkWidget* dialog);
+
 // Parses |button_string| into |leading_buttons| and
 // |trailing_buttons|.  The string is of the format
 // "<button>*:<button*>", for example, "close:minimize:maximize".

chrome/browser/ui/libgtkui/print_dialog_gtk.cc

@@ -180,8 +180,11 @@ PrintDialogGtk::~PrintDialogGtk() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (dialog_) {
-    if (parent_)
-      parent_->RemoveObserver(this);
+    aura::Window* parent = libgtkui::GetAuraTransientParent(dialog_);
+    if (parent) {
+      parent->RemoveObserver(this);
+      libgtkui::ClearAuraTransientParent(dialog_);
+    }
     gtk_widget_destroy(dialog_);
     dialog_ = nullptr;
   }
@@ -311,7 +314,6 @@ void PrintDialogGtk::ShowDialog(
 
   dialog_ = gtk_print_unix_dialog_new(nullptr, nullptr);
   libgtkui::SetGtkTransientForAura(dialog_, parent_view);
-  parent_ = parent_view;
   if (parent_view)
     parent_view->AddObserver(this);
   g_signal_connect(dialog_, "delete-event",
@@ -525,8 +527,9 @@ void PrintDialogGtk::InitPrintSettings(PrintSettings* settings) {
 }
 
 void PrintDialogGtk::OnWindowDestroying(aura::Window* window) {
-  DCHECK_EQ(parent_, window);
-  parent_ = nullptr;
+  DCHECK_EQ(libgtkui::GetAuraTransientParent(dialog_), window);
+
+  libgtkui::ClearAuraTransientParent(dialog_);
   window->RemoveObserver(this);
   if (callback_)
     std::move(callback_).Run(PrintingContextLinux::CANCEL);

chrome/browser/ui/libgtkui/print_dialog_gtk.h

@@ -84,8 +84,6 @@ class PrintDialogGtk : public printing::PrintDialogGtkInterface,
   GtkPageSetup* page_setup_ = nullptr;
   GtkPrinter* printer_ = nullptr;
 
-  aura::Window* parent_ = nullptr;
-
   base::FilePath path_to_pdf_;
 
   DISALLOW_COPY_AND_ASSIGN(PrintDialogGtk);

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk.cc

@@ -62,6 +62,12 @@ void OnFileFilterDataDestroyed(std::string* file_extension) {
   delete file_extension;
 }
 
+// Runs DesktopWindowTreeHostX11::EnableEventListening() when the file-picker
+// is closed.
+void OnFilePickerDestroy(base::Closure* callback) {
+  callback->Run();
+}
+
 }  // namespace
 
 namespace libgtkui {
@@ -86,16 +92,17 @@ SelectFileDialogImplGTK::SelectFileDialogImplGTK(
     : SelectFileDialogImpl(listener, std::move(policy)), preview_(nullptr) {}
 
 SelectFileDialogImplGTK::~SelectFileDialogImplGTK() {
-  while (!dialogs_.empty())
-    DestroyDialog(dialogs_.begin()->first);
+  for (std::set<aura::Window*>::iterator iter = parents_.begin();
+       iter != parents_.end(); ++iter) {
+    (*iter)->RemoveObserver(this);
+  }
+  while (dialogs_.begin() != dialogs_.end()) {
+    gtk_widget_destroy(*(dialogs_.begin()));
+  }
 }
 
 bool SelectFileDialogImplGTK::IsRunning(gfx::NativeWindow parent_window) const {
-  for (auto& pair : dialogs_) {
-    if (pair.second->parent == parent_window)
-      return true;
-  }
-  return false;
+  return parents_.find(parent_window) != parents_.end();
 }
 
 bool SelectFileDialogImplGTK::HasMultipleFileTypeChoicesImpl() {
@@ -104,11 +111,17 @@ bool SelectFileDialogImplGTK::HasMultipleFileTypeChoicesImpl() {
 
 void SelectFileDialogImplGTK::OnWindowDestroying(aura::Window* window) {
   // Remove the |parent| property associated with the |dialog|.
-  for (auto& pair : dialogs_) {
-    if (pair.second->parent == window) {
-      pair.second->parent = nullptr;
-      window->RemoveObserver(this);
+  for (std::set<GtkWidget*>::iterator it = dialogs_.begin();
+       it != dialogs_.end(); ++it) {
+    aura::Window* parent = GetAuraTransientParent(*it);
+    if (parent == window)
+      ClearAuraTransientParent(*it);
   }
+
+  std::set<aura::Window*>::iterator iter = parents_.find(window);
+  if (iter != parents_.end()) {
+    (*iter)->RemoveObserver(this);
+    parents_.erase(iter);
   }
 }
 
@@ -122,12 +135,10 @@ void SelectFileDialogImplGTK::SelectFileImpl(
     const base::FilePath::StringType& default_extension,
     gfx::NativeWindow owning_window,
     void* params) {
-  std::unique_ptr<WidgetData> widget_data = std::make_unique<WidgetData>();
-
   type_ = type;
   if (owning_window) {
     owning_window->AddObserver(this);
-    widget_data->parent = owning_window;
+    parents_.insert(owning_window);
   }
 
   std::string title_string = base::UTF16ToUTF8(title);
@@ -160,6 +171,7 @@ void SelectFileDialogImplGTK::SelectFileImpl(
   }
   g_signal_connect(dialog, "delete-event",
                    G_CALLBACK(gtk_widget_hide_on_delete), nullptr);
+  dialogs_.insert(dialog);
 
   preview_ = gtk_image_new();
   g_signal_connect(dialog, "destroy", G_CALLBACK(OnFileChooserDestroyThunk),
@@ -168,7 +180,7 @@ void SelectFileDialogImplGTK::SelectFileImpl(
                    this);
   gtk_file_chooser_set_preview_widget(GTK_FILE_CHOOSER(dialog), preview_);
 
-  widget_data->params = params;
+  params_map_[dialog] = params;
 
   // Disable input events handling in the host window to make this dialog modal.
   if (owning_window) {
@@ -178,16 +190,20 @@ void SelectFileDialogImplGTK::SelectFileImpl(
       // been captured and by turning off event listening, it is never
       // released. So we manually ensure there is no current capture.
       host->ReleaseCapture();
-      widget_data->enable_event_listening =
+      std::unique_ptr<base::Closure> callback =
           views::DesktopWindowTreeHostX11::GetHostForXID(
               host->GetAcceleratedWidget())
               ->DisableEventListening();
+      // OnFilePickerDestroy() is called when |dialog| destroyed, which allows
+      // to invoke the callback function to re-enable event handling on the
+      // owning window.
+      g_object_set_data_full(
+          G_OBJECT(dialog), "callback", callback.release(),
+          reinterpret_cast<GDestroyNotify>(OnFilePickerDestroy));
       gtk_window_set_modal(GTK_WINDOW(dialog), TRUE);
     }
   }
 
-  dialogs_[dialog] = std::move(widget_data);
-
 #if !GTK_CHECK_VERSION(3, 90, 0)
   gtk_widget_show_all(dialog);
 #endif
@@ -272,9 +288,9 @@ void SelectFileDialogImplGTK::FileSelected(GtkWidget* dialog,
     GSList* filters = gtk_file_chooser_list_filters(GTK_FILE_CHOOSER(dialog));
     int idx = g_slist_index(filters, selected_filter);
     g_slist_free(filters);
-    listener_->FileSelected(path, idx + 1, GetParamsForDialog(dialog));
+    listener_->FileSelected(path, idx + 1, PopParamsForDialog(dialog));
   }
-  DestroyDialog(dialog);
+  gtk_widget_destroy(dialog);
 }
 
 void SelectFileDialogImplGTK::MultiFilesSelected(
@@ -283,15 +299,15 @@ void SelectFileDialogImplGTK::MultiFilesSelected(
   *last_opened_path_ = files[0].DirName();
 
   if (listener_)
-    listener_->MultiFilesSelected(files, GetParamsForDialog(dialog));
-  DestroyDialog(dialog);
+    listener_->MultiFilesSelected(files, PopParamsForDialog(dialog));
+  gtk_widget_destroy(dialog);
 }
 
 void SelectFileDialogImplGTK::FileNotSelected(GtkWidget* dialog) {
-  void* params = GetParamsForDialog(dialog);
+  void* params = PopParamsForDialog(dialog);
   if (listener_)
     listener_->FileSelectionCanceled(params);
-  DestroyDialog(dialog);
+  gtk_widget_destroy(dialog);
 }
 
 GtkWidget* SelectFileDialogImplGTK::CreateFileOpenHelper(
@@ -438,37 +454,12 @@ GtkWidget* SelectFileDialogImplGTK::CreateSaveAsDialog(
   return dialog;
 }
 
-void SelectFileDialogImplGTK::DestroyDialog(GtkWidget* dialog) {
-  if (dialogs_.find(dialog) == dialogs_.end())
-    return;
-
-  // Ensure all signals associated with |dialog| that point back to |this| get
-  // cleaned up so there's no possibility of calling back into |this| after it
-  // gets destroyed.
-  gulong signal = 0;
-  while ((signal = g_signal_handler_find(dialog, G_SIGNAL_MATCH_DATA, 0, 0,
-                                         nullptr, nullptr, this))) {
-    g_signal_handler_disconnect(dialog, signal);
-  }
-
-  gtk_widget_destroy(dialog);
-  OnFileChooserDestroyInternal(dialog);
-}
-
-void SelectFileDialogImplGTK::OnFileChooserDestroyInternal(GtkWidget* dialog) {
-  CHECK(dialog);
-  // |parent| can be nullptr when closing the host window
-  // while opening the file-picker.
-  aura::Window* parent = dialogs_[dialog]->parent;
-  if (parent)
-    parent->RemoveObserver(this);
-  std::move(*dialogs_[dialog]->enable_event_listening).Run();
-  dialogs_.erase(dialog);
-}
-
-void* SelectFileDialogImplGTK::GetParamsForDialog(GtkWidget* dialog) {
-  DCHECK(dialogs_.find(dialog) != dialogs_.end());
-  return dialogs_[dialog]->params;
+void* SelectFileDialogImplGTK::PopParamsForDialog(GtkWidget* dialog) {
+  std::map<GtkWidget*, void*>::iterator iter = params_map_.find(dialog);
+  DCHECK(iter != params_map_.end());
+  void* params = iter->second;
+  params_map_.erase(iter);
+  return params;
 }
 
 bool SelectFileDialogImplGTK::IsCancelResponse(gint response_id) {
@@ -512,20 +503,17 @@ void SelectFileDialogImplGTK::SelectSingleFileHelper(GtkWidget* dialog,
 void SelectFileDialogImplGTK::OnSelectSingleFileDialogResponse(
     GtkWidget* dialog,
     int response_id) {
-  scoped_refptr<SelectFileDialogImplGTK> keep_alive{this};
   SelectSingleFileHelper(dialog, response_id, false);
 }
 
 void SelectFileDialogImplGTK::OnSelectSingleFolderDialogResponse(
     GtkWidget* dialog,
     int response_id) {
-  scoped_refptr<SelectFileDialogImplGTK> keep_alive{this};
   SelectSingleFileHelper(dialog, response_id, true);
 }
 
 void SelectFileDialogImplGTK::OnSelectMultiFileDialogResponse(GtkWidget* dialog,
                                                               int response_id) {
-  scoped_refptr<SelectFileDialogImplGTK> keep_alive{this};
   if (IsCancelResponse(response_id)) {
     FileNotSelected(dialog);
     return;
@@ -555,12 +543,23 @@ void SelectFileDialogImplGTK::OnSelectMultiFileDialogResponse(GtkWidget* dialog,
 }
 
 void SelectFileDialogImplGTK::OnFileChooserDestroy(GtkWidget* dialog) {
-  scoped_refptr<SelectFileDialogImplGTK> keep_alive{this};
-  OnFileChooserDestroyInternal(dialog);
+  dialogs_.erase(dialog);
+
+  // |parent| can be nullptr when closing the host window
+  // while opening the file-picker.
+  aura::Window* parent = GetAuraTransientParent(dialog);
+  if (!parent)
+    return;
+  std::set<aura::Window*>::iterator iter = parents_.find(parent);
+  if (iter != parents_.end()) {
+    (*iter)->RemoveObserver(this);
+    parents_.erase(iter);
+  } else {
+    NOTREACHED();
+  }
 }
 
 void SelectFileDialogImplGTK::OnUpdatePreview(GtkWidget* chooser) {
-  scoped_refptr<SelectFileDialogImplGTK> keep_alive{this};
   gchar* filename =
       gtk_file_chooser_get_preview_filename(GTK_FILE_CHOOSER(chooser));
   if (!filename) {
@@ -591,8 +590,4 @@ void SelectFileDialogImplGTK::OnUpdatePreview(GtkWidget* chooser) {
                                              pixbuf ? TRUE : FALSE);
 }
 
-SelectFileDialogImplGTK::WidgetData::WidgetData() {}
-
-SelectFileDialogImplGTK::WidgetData::~WidgetData() {}
-
 }  // namespace libgtkui

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_UI_LIBGTKUI_SELECT_FILE_DIALOG_IMPL_GTK_H_
 #define CHROME_BROWSER_UI_LIBGTKUI_SELECT_FILE_DIALOG_IMPL_GTK_H_
 
-#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/libgtkui/gtk_util.h"
 #include "chrome/browser/ui/libgtkui/select_file_dialog_impl.h"
@@ -40,20 +39,6 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
 
  private:
   friend class FilePicker;
-
-  struct WidgetData {
-    WidgetData();
-    ~WidgetData();
-
-    // User data that we pass back to |listener_| once the result of the select
-    // file/folder action is known.
-    void* params = nullptr;
-
-    aura::Window* parent = nullptr;
-
-    std::unique_ptr<base::OnceClosure> enable_event_listening;
-  };
-
   bool HasMultipleFileTypeChoicesImpl() override;
 
   // Overridden from aura::WindowObserver:
@@ -91,8 +76,9 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
                                 const base::FilePath& default_path,
                                 gfx::NativeWindow parent);
 
-  // Returns the |params| associated with |dialog|.
-  void* GetParamsForDialog(GtkWidget* dialog);
+  // Removes and returns the |params| associated with |dialog| from
+  // |params_map_|.
+  void* PopParamsForDialog(GtkWidget* dialog);
 
   // Check whether response_id corresponds to the user cancelling/closing the
   // dialog. Used as a helper for the below callbacks.
@@ -109,16 +95,6 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
                                   const base::FilePath& default_path,
                                   gfx::NativeWindow parent);
 
-  // Destroys the widget and deallocates all resources for dialogs_[dialog].
-  void DestroyDialog(GtkWidget* dialog);
-
-  // Deallocates all resources for dialogs_[dialog].
-  void OnFileChooserDestroyInternal(GtkWidget* dialog);
-
-  // The below callbacks may only be called from GTK, otherwise it's possible
-  // that the keep_alive scoped_refptr's will double-destruct |this|
-  // (https://crbug.com/897999).
-
   // Callback for when the user responds to a Save As or Open File dialog.
   CHROMEG_CALLBACK_1(SelectFileDialogImplGTK,
                      void,
@@ -152,11 +128,17 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
                      OnUpdatePreview,
                      GtkWidget*);
 
+  // A map from dialog windows to the |params| user data associated with them.
+  std::map<GtkWidget*, void*> params_map_;
+
   // The GtkImage widget for showing previews of selected images.
   GtkWidget* preview_;
 
   // All our dialogs.
-  base::flat_map<GtkWidget*, std::unique_ptr<WidgetData>> dialogs_;
+  std::set<GtkWidget*> dialogs_;
+
+  // The set of all parent windows for which we are currently running dialogs.
+  std::set<aura::Window*> parents_;
 
   DISALLOW_COPY_AND_ASSIGN(SelectFileDialogImplGTK);
 };

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk_unittest.cc

@@ -55,7 +55,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
         static_cast<SelectFileDialogImplGTK*>(select_file_dialog_.get());
 
     while (!file_dialog->dialogs_.empty())
-      gtk_widget_destroy(file_dialog->dialogs_.begin()->first);
+      gtk_widget_destroy(*(file_dialog->dialogs_.begin()));
 
     select_file_dialog_->ListenerDestroyed();
   }
@@ -85,7 +85,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
   GtkFileChooser* getChooser() {
     auto* dialog =
         static_cast<SelectFileDialogImplGTK*>(select_file_dialog_.get());
-    return GTK_FILE_CHOOSER(dialog->dialogs_.begin()->first);
+    return GTK_FILE_CHOOSER(*(dialog->dialogs_.begin()));
   }
 
   DISALLOW_COPY_AND_ASSIGN(FilePicker);

chrome/browser/ui/libgtkui/select_file_dialog_interactive_uitest.cc

@@ -51,7 +51,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
 
 
     while (!file_dialog->dialogs_.empty())
-      gtk_widget_destroy(file_dialog->dialogs_.begin()->first);
+      gtk_widget_destroy(*(file_dialog->dialogs_.begin()));
   }
 
   // SelectFileDialog::Listener implementation.

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

@@ -2357,7 +2357,7 @@ gfx::Rect DesktopWindowTreeHostX11::ToPixelRect(
   return gfx::ToEnclosingRect(rect_in_pixels);
 }
 
-std::unique_ptr<base::OnceClosure>
+std::unique_ptr<base::Closure>
 DesktopWindowTreeHostX11::DisableEventListening() {
   // Allows to open multiple file-pickers. See https://crbug.com/678982
   modal_dialog_counter_++;
@@ -2368,8 +2368,8 @@ DesktopWindowTreeHostX11::DisableEventListening() {
         window(), std::make_unique<aura::NullWindowTargeter>());
   }
 
-  return std::make_unique<base::OnceClosure>(
-      base::BindOnce(&DesktopWindowTreeHostX11::EnableEventListening,
+  return std::make_unique<base::Closure>(
+      base::Bind(&DesktopWindowTreeHostX11::EnableEventListening,
                  weak_factory_.GetWeakPtr()));
 }
 

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h

@@ -86,7 +86,7 @@ class VIEWS_EXPORT DesktopWindowTreeHostX11
   static void CleanUpWindowList(void (*func)(aura::Window* window));
 
   // Disables event listening to make |dialog| modal.
-  std::unique_ptr<base::OnceClosure> DisableEventListening();
+  std::unique_ptr<base::Closure> DisableEventListening();
 
   // Returns a map of KeyboardEvent code to KeyboardEvent key values.
   base::flat_map<std::string, std::string> GetKeyboardLayoutMap() override;