Make MediaRouterFileDialog hold a WeakPtr to its delegate

MRFileDialog's delegate (MediaRouterViewsUI) may get deleted before
MRFileDialog if the Cast dialog somehow gets closed before the file
selection dialog does. Use a WeakPtr to prevent crashes in this scenario.

Bug: 1019925
Change-Id: Ibcc6e9f8297e5a40777acb87767e127afe04d197
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894970Reviewed-by: Brandon Tolsch <btolsch@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711898}

chrome/browser/ui/media_router/media_router_file_dialog.cc

@@ -123,17 +123,18 @@ void MediaRouterFileDialog::FileSystemDelegate::OpenFileDialog(
 // End of FileSystemDelegate default implementations
 
 MediaRouterFileDialog::MediaRouterFileDialog(
-    MediaRouterFileDialogDelegate* delegate)
-    : MediaRouterFileDialog(delegate, std::make_unique<FileSystemDelegate>()) {}
+    base::WeakPtr<MediaRouterFileDialogDelegate> delegate)
+    : MediaRouterFileDialog(std::move(delegate),
+                            std::make_unique<FileSystemDelegate>()) {}
 
 // Used for tests
 MediaRouterFileDialog::MediaRouterFileDialog(
-    MediaRouterFileDialogDelegate* delegate,
+    base::WeakPtr<MediaRouterFileDialogDelegate> delegate,
     std::unique_ptr<FileSystemDelegate> file_system_delegate)
     : task_runner_(base::CreateTaskRunner({base::ThreadPool(), base::MayBlock(),
                                            base::TaskPriority::USER_VISIBLE})),
       file_system_delegate_(std::move(file_system_delegate)),
-      delegate_(delegate) {}
+      delegate_(std::move(delegate)) {}
 
 MediaRouterFileDialog::~MediaRouterFileDialog() = default;
 
@@ -224,15 +225,17 @@ void MediaRouterFileDialog::OnValidationResults(
     MediaRouterFileDialog::ValidationResult validation_result) {
   if (validation_result == MediaRouterFileDialog::FILE_OK) {
     selected_file_ = file_info;
-    delegate_->FileDialogFileSelected(file_info);
-  } else {
+    if (delegate_)
+      delegate_->FileDialogFileSelected(file_info);
+  } else if (delegate_) {
     delegate_->FileDialogSelectionFailed(
         CreateIssue(file_info, validation_result));
   }
 }
 
 void MediaRouterFileDialog::FileSelectionCanceled(void* params) {
-  delegate_->FileDialogSelectionCanceled();
+  if (delegate_)
+    delegate_->FileDialogSelectionCanceled();
 }
 
 IssueInfo MediaRouterFileDialog::CreateIssue(

chrome/browser/ui/media_router/media_router_file_dialog.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_MEDIA_ROUTER_MEDIA_ROUTER_FILE_DIALOG_H_
 
 #include "base/files/file_util.h"
+#include "base/memory/weak_ptr.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/media_router/issue.h"
@@ -91,11 +92,12 @@ class MediaRouterFileDialog : public ui::SelectFileDialog::Listener {
     scoped_refptr<ui::SelectFileDialog> select_file_dialog_;
   };
 
-  explicit MediaRouterFileDialog(MediaRouterFileDialogDelegate* delegate);
+  explicit MediaRouterFileDialog(
+      base::WeakPtr<MediaRouterFileDialogDelegate> delegate);
 
   // Constuctor with injectable FileSystemDelegate, used for tests.
   MediaRouterFileDialog(
-      MediaRouterFileDialogDelegate* delegate,
+      base::WeakPtr<MediaRouterFileDialogDelegate> delegate,
       std::unique_ptr<FileSystemDelegate> file_system_delegate);
 
   ~MediaRouterFileDialog() override;
@@ -152,7 +154,7 @@ class MediaRouterFileDialog : public ui::SelectFileDialog::Listener {
   std::unique_ptr<FileSystemDelegate> file_system_delegate_;
 
   // Object which the media router file dialog callbacks get sent to.
-  MediaRouterFileDialogDelegate* const delegate_;
+  base::WeakPtr<MediaRouterFileDialogDelegate> const delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaRouterFileDialog);
 };

chrome/browser/ui/media_router/media_router_file_dialog_unittest.cc

@@ -42,6 +42,13 @@ class MockDelegate
                void(const ui::SelectedFileInfo& file_info));
   MOCK_METHOD1(FileDialogSelectionFailed, void(const IssueInfo& issue));
   MOCK_METHOD0(FileDialogSelectionCanceled, void());
+
+  base::WeakPtr<MockDelegate> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  base::WeakPtrFactory<MockDelegate> weak_factory_{this};
 };
 
 class MockFileSystemDelegate
@@ -76,8 +83,8 @@ class MediaRouterFileDialogTest : public Test {
     auto temp_mock = std::make_unique<MockFileSystemDelegate>();
     mock_file_system_delegate = temp_mock.get();
 
-    dialog_ = std::make_unique<MediaRouterFileDialog>(mock_delegate_.get(),
-                                                      std::move(temp_mock));
+    dialog_ = std::make_unique<MediaRouterFileDialog>(
+        mock_delegate_->GetWeakPtr(), std::move(temp_mock));
     dialog_as_listener_ = dialog_.get();
 
     // Setup default file checks to all pass

chrome/browser/ui/views/media_router/media_router_views_ui.cc

@@ -403,7 +403,8 @@ void MediaRouterViewsUI::RemoveIssue(const Issue::Id& issue_id) {
 
 void MediaRouterViewsUI::OpenFileDialog() {
   if (!media_router_file_dialog_) {
-    media_router_file_dialog_ = std::make_unique<MediaRouterFileDialog>(this);
+    media_router_file_dialog_ =
+        std::make_unique<MediaRouterFileDialog>(weak_factory_.GetWeakPtr());
   }
 
   media_router_file_dialog_->OpenFileDialog(GetBrowser());