FileInputType should use WeakPtr for FileListCreator lambdas

https://bugs.webkit.org/show_bug.cgi?id=213130 <rdar://problem/64276591> Reviewed by David Kilzer. FileInputType::filesChosen was passing a completion handler to FileListCreator::create that captured |this|. If the FileListCreator instance still existed when |this| was destroyed, FileInputType::~FileInputType would clear the captured |this| by calling FileListCreator::clear. This can be simplified by having the FileListCreator completion handler capture a WeakPtr to |this|. Also, when FileInputType::allowsDirectories is false, m_fileListCreator would not be properly cleared after creating the file list. The FileListCreator completion handler would set m_fileListCreator to nullptr, but would be executed *before* FileListCreator::create returned and set m_fileListCreator to the newly-created FileListCreator object. Fixed this by having FileListCreator::create execute the completion handler immediately and return nullptr in cases where a FileListCreator does not need to be created for directory resolution. Covered by existing tests. * html/FileInputType.cpp: (WebCore::FileInputType::~FileInputType): (WebCore::FileInputType::filesChosen): * html/FileInputType.h: * html/FileListCreator.cpp: (WebCore::createFileList): (WebCore::FileListCreator::create): (WebCore::FileListCreator::FileListCreator): (WebCore::FileListCreator::createFileList): * html/FileListCreator.h: (WebCore::FileListCreator::create): Deleted. Canonical link: https://commits.webkit.org/225916@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@262962 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 12, 2020 · c481f5eb76ce21b309f4f4e88d5bc19808ecdc99 · c481f5e
1 parent 53f7c03
commit c481f5eb76ce21b309f4f4e88d5bc19808ecdc99
Showing 5 changed files with 77 additions and 40 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,39 @@
+2020-06-12  Andy Estes  <aestes@apple.com>
+
+        FileInputType should use WeakPtr for FileListCreator lambdas
+        https://bugs.webkit.org/show_bug.cgi?id=213130
+        <rdar://problem/64276591>
+
+        Reviewed by David Kilzer.
+
+        FileInputType::filesChosen was passing a completion handler to FileListCreator::create that
+        captured |this|. If the FileListCreator instance still existed when |this| was destroyed,
+        FileInputType::~FileInputType would clear the captured |this| by calling
+        FileListCreator::clear. This can be simplified by having the FileListCreator completion
+        handler capture a WeakPtr to |this|.
+
+        Also, when FileInputType::allowsDirectories is false, m_fileListCreator would not be
+        properly cleared after creating the file list. The FileListCreator completion handler would
+        set m_fileListCreator to nullptr, but would be executed *before* FileListCreator::create
+        returned and set m_fileListCreator to the newly-created FileListCreator object. Fixed this
+        by having FileListCreator::create execute the completion handler immediately and return
+        nullptr in cases where a FileListCreator does not need to be created for directory
+        resolution.
+
+        Covered by existing tests.
+
+        * html/FileInputType.cpp:
+        (WebCore::FileInputType::~FileInputType):
+        (WebCore::FileInputType::filesChosen):
+        * html/FileInputType.h:
+        * html/FileListCreator.cpp:
+        (WebCore::createFileList):
+        (WebCore::FileListCreator::create):
+        (WebCore::FileListCreator::FileListCreator):
+        (WebCore::FileListCreator::createFileList):
+        * html/FileListCreator.h:
+        (WebCore::FileListCreator::create): Deleted.
+
 2020-06-12  Antti Koivisto  <antti@apple.com>
 
         REGRESSION (r262618): Very slow typing in a github issue

diff --git a/Source/WebCore/html/FileInputType.cpp b/Source/WebCore/html/FileInputType.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2020 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
@@ -103,9 +103,6 @@ FileInputType::FileInputType(HTMLInputElement& element)
 
 FileInputType::~FileInputType()
 {
-    if (m_fileListCreator)
-        m_fileListCreator->cancel();
-
     if (m_fileChooser)
         m_fileChooser->invalidate();
 
@@ -416,7 +413,10 @@ void FileInputType::filesChosen(const Vector<FileChooserFileInfo>& paths, const
         m_fileListCreator->cancel();
 
     auto shouldResolveDirectories = allowsDirectories() ? FileListCreator::ShouldResolveDirectories::Yes : FileListCreator::ShouldResolveDirectories::No;
-    m_fileListCreator = FileListCreator::create(paths, shouldResolveDirectories, [this, icon = makeRefPtr(icon)](Ref<FileList>&& fileList) mutable {
+    m_fileListCreator = FileListCreator::create(paths, shouldResolveDirectories, [this, weakThis = makeWeakPtr(*this), icon = makeRefPtr(icon)](Ref<FileList>&& fileList) mutable {
+        auto protectedThis = makeRefPtr(weakThis.get());
+        if (!protectedThis)
+            return;
         setFiles(WTFMove(fileList), icon ? RequestIcon::Yes : RequestIcon::No);
         if (icon && !m_fileList->isEmpty() && element())
             iconLoaded(WTFMove(icon));

diff --git a/Source/WebCore/html/FileInputType.h b/Source/WebCore/html/FileInputType.h
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
- * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@
 #include "FileChooser.h"
 #include "FileIconLoader.h"
 #include <wtf/RefPtr.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
@@ -43,7 +44,7 @@ class FileList;
 class FileListCreator;
 class Icon;
 
-class FileInputType final : public BaseClickableWithKeyInputType, private FileChooserClient, private FileIconLoaderClient {
+class FileInputType final : public BaseClickableWithKeyInputType, private FileChooserClient, private FileIconLoaderClient, public CanMakeWeakPtr<FileInputType> {
 public:
     explicit FileInputType(HTMLInputElement&);
     virtual ~FileInputType();

diff --git a/Source/WebCore/html/FileListCreator.cpp b/Source/WebCore/html/FileListCreator.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -36,7 +36,7 @@ namespace WebCore {
 
 FileListCreator::~FileListCreator()
 {
-    ASSERT(!m_completionHander);
+    ASSERT(!m_completionHandler);
 }
 
 static void appendDirectoryFiles(const String& directory, const String& relativePath, Vector<Ref<File>>& fileObjects)
@@ -57,40 +57,46 @@ static void appendDirectoryFiles(const String& directory, const String& relative
     }
 }
 
-FileListCreator::FileListCreator(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
-{
-    if (shouldResolveDirectories == ShouldResolveDirectories::No)
-        completionHandler(createFileList<ShouldResolveDirectories::No>(paths));
-    else {
-        // Resolve directories on a background thread to avoid blocking the main thread.
-        m_completionHander = WTFMove(completionHandler);
-        m_workQueue = WorkQueue::create("FileListCreator Work Queue");
-        m_workQueue->dispatch([this, protectedThis = makeRef(*this), paths = crossThreadCopy(paths)]() mutable {
-            auto fileList = createFileList<ShouldResolveDirectories::Yes>(paths);
-            callOnMainThread([this, protectedThis = WTFMove(protectedThis), fileList = WTFMove(fileList)]() mutable {
-                if (auto completionHander = WTFMove(m_completionHander))
-                    completionHander(WTFMove(fileList));
-            });
-        });
-    }
-}
-
 template<FileListCreator::ShouldResolveDirectories shouldResolveDirectories>
-Ref<FileList> FileListCreator::createFileList(const Vector<FileChooserFileInfo>& paths)
+static Ref<FileList> createFileList(const Vector<FileChooserFileInfo>& paths)
 {
     Vector<Ref<File>> fileObjects;
     for (auto& info : paths) {
-        if (shouldResolveDirectories == ShouldResolveDirectories::Yes && FileSystem::fileIsDirectory(info.path, FileSystem::ShouldFollowSymbolicLinks::No))
+        if (shouldResolveDirectories == FileListCreator::ShouldResolveDirectories::Yes && FileSystem::fileIsDirectory(info.path, FileSystem::ShouldFollowSymbolicLinks::No))
             appendDirectoryFiles(info.path, FileSystem::pathGetFileName(info.path), fileObjects);
         else
             fileObjects.append(File::create(info.path, info.displayName));
     }
     return FileList::create(WTFMove(fileObjects));
 }
 
+RefPtr<FileListCreator> FileListCreator::create(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
+{
+    if (shouldResolveDirectories == ShouldResolveDirectories::No) {
+        completionHandler(createFileList<ShouldResolveDirectories::No>(paths));
+        return nullptr;
+    }
+
+    return adoptRef(*new FileListCreator(paths, WTFMove(completionHandler)));
+}
+
+FileListCreator::FileListCreator(const Vector<FileChooserFileInfo>& paths, CompletionHandler&& completionHandler)
+    : m_workQueue(WorkQueue::create("FileListCreator Work Queue"))
+    , m_completionHandler(WTFMove(completionHandler))
+{
+    // Resolve directories on a background thread to avoid blocking the main thread.
+    m_workQueue->dispatch([this, protectedThis = makeRef(*this), paths = crossThreadCopy(paths)]() mutable {
+        auto fileList = createFileList<ShouldResolveDirectories::Yes>(paths);
+        callOnMainThread([this, protectedThis = WTFMove(protectedThis), fileList = WTFMove(fileList)]() mutable {
+            if (auto completionHandler = WTFMove(m_completionHandler))
+                completionHandler(WTFMove(fileList));
+        });
+    });
+}
+
 void FileListCreator::cancel()
 {
-    m_completionHander = nullptr;
+    m_completionHandler = nullptr;
     m_workQueue = nullptr;
 }
 

diff --git a/Source/WebCore/html/FileListCreator.h b/Source/WebCore/html/FileListCreator.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -41,23 +41,17 @@ class FileListCreator : public ThreadSafeRefCounted<FileListCreator> {
     using CompletionHandler = Function<void(Ref<FileList>&&)>;
 
     enum class ShouldResolveDirectories { No, Yes };
-    static Ref<FileListCreator> create(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
-    {
-        return adoptRef(*new FileListCreator(paths, shouldResolveDirectories, WTFMove(completionHandler)));
-    }
+    static RefPtr<FileListCreator> create(const Vector<FileChooserFileInfo>&, ShouldResolveDirectories, CompletionHandler&&);
 
     ~FileListCreator();
 
     void cancel();
 
 private:
-    FileListCreator(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories, CompletionHandler&&);
-
-    template<ShouldResolveDirectories shouldResolveDirectories>
-    static Ref<FileList> createFileList(const Vector<FileChooserFileInfo>&);
+    FileListCreator(const Vector<FileChooserFileInfo>&, CompletionHandler&&);
 
     RefPtr<WorkQueue> m_workQueue;
-    CompletionHandler m_completionHander;
+    CompletionHandler m_completionHandler;
 };
 
 }