Update FileSelectHelper to use RenderFrameHost notifications.

The FileSelectHelper class is using RenderViewHost notifications to
ensure it cleans up itself. However, as I moved it to use
RenderFrameHost for file selection (but not directory enumeration yet),
it opened it up to use-after-free conditions as it was not monitoring
for those objects going away.

This CL fixes that and adds a test to ensure it gets properly cleaned up
when an iframe that requested the file chooser is removed.

BUG=621076

Review-Url: https://codereview.chromium.org/2102883002
Cr-Commit-Position: refs/heads/master@{#402683}

chrome/browser/file_select_helper.cc

@@ -198,7 +198,7 @@ void FileSelectHelper::FileSelectedWithExtraInfo(
       FROM_HERE,
       base::Bind(&FileSelectHelper::ProcessSelectedFilesMac, this, files));
 #else
-  NotifyRenderViewHostAndEnd(files);
+  NotifyRenderFrameHostAndEnd(files);
 #endif  // defined(OS_MACOSX)
 }
 
@@ -223,12 +223,12 @@ void FileSelectHelper::MultiFilesSelectedWithExtraInfo(
       FROM_HERE,
       base::Bind(&FileSelectHelper::ProcessSelectedFilesMac, this, files));
 #else
-  NotifyRenderViewHostAndEnd(files);
+  NotifyRenderFrameHostAndEnd(files);
 #endif  // defined(OS_MACOSX)
 }
 
 void FileSelectHelper::FileSelectionCanceled(void* params) {
-  NotifyRenderViewHostAndEnd(std::vector<ui::SelectedFileInfo>());
+  NotifyRenderFrameHostAndEnd(std::vector<ui::SelectedFileInfo>());
 }
 
 void FileSelectHelper::StartNewEnumeration(const base::FilePath& path,
@@ -279,14 +279,14 @@ void FileSelectHelper::OnListDone(int id, int error) {
       FilePathListToSelectedFileInfoList(entry->results_);
 
   if (id == kFileSelectEnumerationId) {
-    NotifyRenderViewHostAndEnd(selected_files);
+    NotifyRenderFrameHostAndEnd(selected_files);
   } else {
     entry->rvh_->DirectoryEnumerationFinished(id, entry->results_);
     EnumerateDirectoryEnd();
   }
 }
 
-void FileSelectHelper::NotifyRenderViewHostAndEnd(
+void FileSelectHelper::NotifyRenderFrameHostAndEnd(
     const std::vector<ui::SelectedFileInfo>& files) {
   if (!render_frame_host_) {
     RunFileChooserEnd();
@@ -308,8 +308,9 @@ void FileSelectHelper::NotifyRenderViewHostAndEnd(
             ->GetFileSystemContext();
     file_manager::util::ConvertSelectedFileInfoListToFileChooserFileInfoList(
         file_system_context, site_instance->GetSiteURL(), files,
-        base::Bind(&FileSelectHelper::NotifyRenderViewHostAndEndAfterConversion,
-                   this));
+        base::Bind(
+            &FileSelectHelper::NotifyRenderFrameHostAndEndAfterConversion,
+            this));
     return;
   }
 #endif  // defined(OS_CHROMEOS)
@@ -322,10 +323,10 @@ void FileSelectHelper::NotifyRenderViewHostAndEnd(
     chooser_files.push_back(chooser_file);
   }
 
-  NotifyRenderViewHostAndEndAfterConversion(chooser_files);
+  NotifyRenderFrameHostAndEndAfterConversion(chooser_files);
 }
 
-void FileSelectHelper::NotifyRenderViewHostAndEndAfterConversion(
+void FileSelectHelper::NotifyRenderFrameHostAndEndAfterConversion(
     const std::vector<content::FileChooserFileInfo>& list) {
   if (render_frame_host_)
     render_frame_host_->FilesSelectedInChooser(list, dialog_mode_);
@@ -341,7 +342,7 @@ void FileSelectHelper::DeleteTemporaryFiles() {
   temporary_files_.clear();
 }
 
-void FileSelectHelper::CleanUpOnRenderViewHostChange() {
+void FileSelectHelper::CleanUp() {
   if (!temporary_files_.empty()) {
     DeleteTemporaryFiles();
 
@@ -541,7 +542,7 @@ void FileSelectHelper::ProceedWithSafeBrowsingVerdict(
     std::unique_ptr<content::FileChooserParams> params,
     bool allowed_by_safe_browsing) {
   if (!allowed_by_safe_browsing) {
-    NotifyRenderViewHostAndEnd(std::vector<ui::SelectedFileInfo>());
+    NotifyRenderFrameHostAndEnd(std::vector<ui::SelectedFileInfo>());
     return;
   }
   RunFileChooserOnUIThread(default_file_path, std::move(params));
@@ -648,19 +649,24 @@ void FileSelectHelper::Observe(int type,
                                const content::NotificationSource& source,
                                const content::NotificationDetails& details) {
   DCHECK_EQ(content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED, type);
-  DCHECK_EQ(content::Source<RenderWidgetHost>(source).ptr(),
-            render_frame_host_->GetRenderViewHost()->GetWidget());
   render_frame_host_ = nullptr;
 }
 
-void FileSelectHelper::RenderViewHostChanged(RenderViewHost* old_host,
-                                             RenderViewHost* new_host) {
-  CleanUpOnRenderViewHostChange();
+void FileSelectHelper::RenderFrameHostChanged(
+    content::RenderFrameHost* old_host,
+    content::RenderFrameHost* new_host) {
+  render_frame_host_ = nullptr;
+}
+
+void FileSelectHelper::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  render_frame_host_ = nullptr;
 }
 
 void FileSelectHelper::WebContentsDestroyed() {
+  render_frame_host_ = nullptr;
   web_contents_ = nullptr;
-  CleanUpOnRenderViewHostChange();
+  CleanUp();
 }
 
 // static

chrome/browser/file_select_helper.h

@@ -33,9 +33,9 @@ namespace ui {
 struct SelectedFileInfo;
 }
 
-// This class handles file-selection requests coming from WebUI elements
-// (via the extensions::ExtensionHost class). It implements both the
-// initialisation and listener functions for file-selection dialogs.
+// This class handles file-selection requests coming from renderer processes.
+// It implements both the initialisation and listener functions for
+// file-selection dialogs.
 //
 // Since FileSelectHelper has-a NotificationRegistrar, it needs to live on and
 // be destroyed on the UI thread. References to FileSelectHelper may be passed
@@ -132,8 +132,9 @@ class FileSelectHelper : public base::RefCountedThreadSafe<
                const content::NotificationDetails& details) override;
 
   // content::WebContentsObserver overrides.
-  void RenderViewHostChanged(content::RenderViewHost* old_host,
-                             content::RenderViewHost* new_host) override;
+  void RenderFrameHostChanged(content::RenderFrameHost* old_host,
+                              content::RenderFrameHost* new_host) override;
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
   void WebContentsDestroyed() override;
 
   void EnumerateDirectory(int request_id,
@@ -173,21 +174,21 @@ class FileSelectHelper : public base::RefCountedThreadSafe<
   static base::FilePath ZipPackage(const base::FilePath& path);
 #endif  // defined(OS_MACOSX)
 
-  // Utility method that passes |files| to the render view host, and ends the
+  // Utility method that passes |files| to the RenderFrameHost, and ends the
   // file chooser.
-  void NotifyRenderViewHostAndEnd(
+  void NotifyRenderFrameHostAndEnd(
       const std::vector<ui::SelectedFileInfo>& files);
 
   // Sends the result to the render process, and call |RunFileChooserEnd|.
-  void NotifyRenderViewHostAndEndAfterConversion(
+  void NotifyRenderFrameHostAndEndAfterConversion(
       const std::vector<content::FileChooserFileInfo>& list);
 
   // Schedules the deletion of the files in |temporary_files_| and clears the
   // vector.
   void DeleteTemporaryFiles();
 
-  // Cleans up when the RenderViewHost of our WebContents changes.
-  void CleanUpOnRenderViewHostChange();
+  // Cleans up when the initiator of the file chooser is no longer valid.
+  void CleanUp();
 
   // Helper method to get allowed extensions for select file dialog from
   // the specified accept types as defined in the spec:

chrome/browser/file_select_helper_mac.mm

@@ -138,5 +138,5 @@ void FileSelectHelper::ProcessSelectedFilesMacOnUIThread(
     }
   }
 
-  NotifyRenderViewHostAndEnd(files);
+  NotifyRenderFrameHostAndEnd(files);
 }

chrome/browser/iframe_browsertest.cc

@@ -8,10 +8,17 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
 #include "url/gurl.h"
 
 class IFrameTest : public InProcessBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
  protected:
   void NavigateAndVerifyTitle(const char* file, const char* page_title) {
     GURL url = ui_test_utils::GetTestUrl(
@@ -30,3 +37,37 @@ IN_PROC_BROWSER_TEST_F(IFrameTest, Crash) {
 IN_PROC_BROWSER_TEST_F(IFrameTest, InEmptyFrame) {
   NavigateAndVerifyTitle("iframe_in_empty_frame.html", "iframe test");
 }
+
+// Test for https://crbug.com/621076. It ensures that file chooser triggered
+// by an iframe, which is destroyed before the chooser is closed, does not
+// result in a use-after-free condition.
+// Note: This test is disabled temporarily to track down a memory leak reported
+// by the ASan bots. It will be enabled once the root cause is found.
+IN_PROC_BROWSER_TEST_F(IFrameTest, DISABLED_FileChooserInDestroyedSubframe) {
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  GURL file_input_url(embedded_test_server()->GetURL("/file_input.html"));
+
+  // Navigate to a page, which contains an iframe, and navigate the iframe
+  // to a document containing a file input field.
+  // Note: For the bug to occur, the parent and child frame need to be in
+  // the same site, otherwise they would each get a RenderWidgetHost and
+  // existing code will properly clear the internal state.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/iframe.html"));
+  NavigateIframeToURL(tab, "test", file_input_url);
+
+  // Invoke the file chooser and remove the iframe from the main document.
+  content::RenderFrameHost* frame = ChildFrameAt(tab->GetMainFrame(), 0);
+  EXPECT_TRUE(frame);
+  EXPECT_EQ(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance());
+  EXPECT_TRUE(
+      ExecuteScript(frame, "document.getElementById('fileinput').click();"));
+  EXPECT_TRUE(ExecuteScript(tab->GetMainFrame(),
+                            "document.body.removeChild("
+                            "document.querySelectorAll('iframe')[0])"));
+  ASSERT_EQ(nullptr, ChildFrameAt(tab->GetMainFrame(), 0));
+
+  // On ASan bots, this test should succeed without reporting use-after-free
+  // condition.
+}

chrome/test/data/file_input.html

+<input type="file" id="fileinput" />