Fix a ASAN crash when dragging a download file on windows

Here is how the crash can happen:
1. DragObjectImpl::GetData() calls DragDownloadFile::Start()
   to start a download.
2. GetData() then calls DragDownloadFile::Wait().
3. DragDownloadFile::Wait() runs the nested_loop_.
4. DragDownloadFile::DownloadCompleted() gets called, which
   calls DataObjectImpl::OnDownloadCompleted(). Inside that
   method, it creates a new StoredDataInfo objects and deletes
   the downloader pointer, which is the DragDownloadFile
   instance.
5. DragDownloadFile dtor is invoked, it will destroy the
   nested_loop_ member variable. However, because RunLoop::Run()
   is called earlier and Wait() is still pending, deleting
   nested_loop_ will hit the DCHECK(!running_) in RunLoop dtor
   and fail.

The issue happens mainly due to step 4, as
DragDownloadFile::DownloadCompleted() could delete itself.
This CL fixes the issue by not deleting the StoredDataInfo object
in step 4, instead, the DragDownloadFile instance is deleted after
the newly constructed StoredDataInfo object in 4 is destroyed.

BUG=968303

Change-Id: I57dbe275f76fd8e566185538a8f13def476ff9f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904950Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715470}

content/browser/download/drag_download_file.cc

@@ -228,13 +228,11 @@ void DragDownloadFile::Start(ui::DownloadFileObserver* observer) {
 
 bool DragDownloadFile::Wait() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // Store the weakptr in a local variable as |this| may be deleted while
-  // waiting for the nested RunLoop.
-  auto ref = weak_ptr_factory_.GetWeakPtr();
+  auto weak_ptr = weak_ptr_factory_.GetWeakPtr();
   if (state_ == STARTED)
     nested_loop_.Run();
-  // If the weakptr is destroyed, the download should be successful.
-  return !ref.get() || state_ == SUCCESS;
+  DCHECK(weak_ptr);
+  return state_ == SUCCESS;
 }
 
 void DragDownloadFile::Stop() {
@@ -257,14 +255,10 @@ void DragDownloadFile::DownloadCompleted(bool is_successful) {
   if (nested_loop_.running())
     nested_loop_.Quit();
 
-  // Calling file_observer->OnDownloadCompleted() could delete this
-  // object.
   if (is_successful)
     file_observer->OnDownloadCompleted(file_path_);
   else
     file_observer->OnDownloadAborted();
-
-  // Nothing should be called here as the object might get deleted.
 }
 
 }  // namespace content

ui/base/dragdrop/os_exchange_data_provider_win.cc

@@ -891,12 +891,16 @@ void DataObjectImpl::RemoveData(const FORMATETC& format) {
 void DataObjectImpl::OnDownloadCompleted(const base::FilePath& file_path) {
   for (std::unique_ptr<StoredDataInfo>& content : contents_) {
     if (content->format_etc.cfFormat == CF_HDROP) {
+      // Retrieve the downloader first so it won't get destroyed.
+      auto downloader = std::move(content->downloader);
+      if (downloader)
+        downloader->Stop();
       // Replace stored data.
       STGMEDIUM* storage =
           GetStorageForFileNames({FileInfo(file_path, base::FilePath())});
       content = std::make_unique<StoredDataInfo>(
           ClipboardFormatType::GetCFHDropType().ToFormatEtc(), storage);
-
+      content->downloader = std::move(downloader);
       break;
     }
   }

ui/base/dragdrop/os_exchange_data_win_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/task_environment.h"
 #include "base/win/scoped_hglobal.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 #include "ui/base/clipboard/clipboard_format_type.h"
@@ -862,4 +863,41 @@ TEST_F(OSExchangeDataWinTest, ProvideURLForPlainTextURL) {
   EXPECT_EQ(GURL("http://google.com"), read_url);
 }
 
+class MockDownloadFileProvider : public ui::DownloadFileProvider {
+ public:
+  MockDownloadFileProvider() = default;
+  ~MockDownloadFileProvider() override = default;
+  base::WeakPtr<MockDownloadFileProvider> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+  MOCK_METHOD1(Start, void(DownloadFileObserver* observer));
+  MOCK_METHOD0(Wait, bool());
+  MOCK_METHOD0(Stop, void());
+
+ private:
+  base::WeakPtrFactory<MockDownloadFileProvider> weak_ptr_factory_{this};
+};
+
+// Verifies that DataObjectImpl::OnDownloadCompleted() doesn't delete
+// the DownloadFileProvider instance.
+TEST_F(OSExchangeDataWinTest, OnDownloadCompleted) {
+  OSExchangeData data;
+  Microsoft::WRL::ComPtr<IDataObject> com_data(
+      OSExchangeDataProviderWin::GetIDataObject(data));
+
+  OSExchangeDataProviderWin provider(com_data.Get());
+
+  auto download_file_provider = std::make_unique<MockDownloadFileProvider>();
+  auto weak_ptr = download_file_provider->GetWeakPtr();
+  OSExchangeData::DownloadFileInfo file_info(
+      base::FilePath(FILE_PATH_LITERAL("file_with_no_contents.txt")),
+      std::move(download_file_provider));
+  provider.SetDownloadFileInfo(&file_info);
+
+  OSExchangeDataProviderWin::GetDataObjectImpl(data)->OnDownloadCompleted(
+      base::FilePath());
+  EXPECT_TRUE(weak_ptr);
+}
+
 }  // namespace ui