Fix an use-after-free issue when dragging a download on windows

The use-after-free is introduced in the following call sequence:
1. DataObjectImpl::GetData() calls DragDownloadFile::Wait(),
  which starts nested runloop and wait for download to complete.
2. DragDownloadFile::DownloadCompleted() is called, and it calls
   DataObjectImpl::OnDownloadCompleted().
3. OnDownloadCompleted() creates a new StoredDataInfo object,
   that clears up the scoped_refptr of DragDownloadFile in the
   StoredDataInfo. As a result, DragDownloadFile is deleted.
4. The nested runloop started in DragDownloadFile::Wait() should
   finish now, but the return statement touches the state_
   member variable on the deleted object,

This CL breaks the above sequence by checking the weakptr in 4 first
before accessing the state_ member variable

BUG=968303

Change-Id: Ie1266d344fe102f73451bda9f996e29d9bd00a31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1827488Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700914}

content/browser/download/drag_download_file.cc

@@ -239,9 +239,13 @@ void DragDownloadFile::Start(ui::DownloadFileObserver* observer) {
 
 bool DragDownloadFile::Wait() {
   CheckThread();
+  // Store the weakptr in a local variable as |this| may be deleted while
+  // waiting for the nested RunLoop.
+  auto ref = weak_ptr_factory_.GetWeakPtr();
   if (state_ == STARTED)
     nested_loop_.Run();
-  return state_ == SUCCESS;
+  // If the weakptr is destroyed, the download should be successful.
+  return !ref.get() || state_ == SUCCESS;
 }
 
 void DragDownloadFile::Stop() {

ui/base/dragdrop/os_exchange_data_provider_win.cc

@@ -842,8 +842,7 @@ static void DuplicateMedium(CLIPFORMAT source_clipformat,
 
 DataObjectImpl::StoredDataInfo::StoredDataInfo(const FORMATETC& format_etc,
                                                STGMEDIUM* medium)
-    : format_etc(format_etc), medium(medium), owns_medium(true) {
-}
+    : format_etc(format_etc), medium(medium), owns_medium(true) {}
 
 DataObjectImpl::StoredDataInfo::~StoredDataInfo() {
   if (owns_medium) {
@@ -920,43 +919,41 @@ HRESULT DataObjectImpl::GetData(FORMATETC* format_etc, STGMEDIUM* medium) {
       // If medium is NULL, delay-rendering will be used.
       if (content->medium) {
         DuplicateMedium(content->format_etc.cfFormat, content->medium, medium);
-      } else {
-        // Fail all GetData() attempts for DownloadURL data if the drag and drop
-        // operation is still in progress.
-        if (in_drag_loop_)
-          return DV_E_FORMATETC;
-
-        bool wait_for_data = false;
-
-        // In async mode, we do not want to start waiting for the data before
-        // the async operation is started. This is because we want to postpone
-        // until Shell kicks off a background thread to do the work so that
-        // we do not block the UI thread.
-        if (!in_async_mode_ || async_operation_started_)
-          wait_for_data = true;
-
-        if (!wait_for_data)
+        return S_OK;
+      }
+      // Fail all GetData() attempts for DownloadURL data if the drag and drop
+      // operation is still in progress.
+      if (in_drag_loop_)
+        return DV_E_FORMATETC;
+
+      bool wait_for_data = false;
+
+      // In async mode, we do not want to start waiting for the data before
+      // the async operation is started. This is because we want to postpone
+      // until Shell kicks off a background thread to do the work so that
+      // we do not block the UI thread.
+      if (!in_async_mode_ || async_operation_started_)
+        wait_for_data = true;
+
+      if (!wait_for_data)
+        return DV_E_FORMATETC;
+
+      // Notify the observer we start waiting for the data. This gives
+      // an observer a chance to end the drag and drop.
+      if (observer_)
+        observer_->OnWaitForData();
+
+      // Now we can start the download.
+      if (content->downloader.get()) {
+        content->downloader->Start(this);
+        if (!content->downloader->Wait()) {
+          is_aborting_ = true;
           return DV_E_FORMATETC;
-
-        // Notify the observer we start waiting for the data. This gives
-        // an observer a chance to end the drag and drop.
-        if (observer_)
-          observer_->OnWaitForData();
-
-        // Now we can start the download.
-        if (content->downloader.get()) {
-          content->downloader->Start(this);
-          if (!content->downloader->Wait()) {
-            is_aborting_ = true;
-            return DV_E_FORMATETC;
-          }
         }
-
-        // The stored data should have been updated with the final version.
-        // So we just need to call this function again to retrieve it.
-        return GetData(format_etc, medium);
       }
-      return S_OK;
+      // The stored data should have been updated with the final version.
+      // So we just need to call this function again to retrieve it.
+      return GetData(format_etc, medium);
     }
   }