- Revision
- 262962
- Author
- aes...@apple.com
- Date
- 2020-06-12 11:40:25 -0700 (Fri, 12 Jun 2020)
Log Message
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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (262961 => 262962)
--- trunk/Source/WebCore/ChangeLog 2020-06-12 18:24:02 UTC (rev 262961)
+++ trunk/Source/WebCore/ChangeLog 2020-06-12 18:40:25 UTC (rev 262962)
@@ -1,3 +1,39 @@
+2020-06-12 Andy Estes <aes...@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 <an...@apple.com>
REGRESSION (r262618): Very slow typing in a github issue
Modified: trunk/Source/WebCore/html/FileInputType.cpp (262961 => 262962)
--- trunk/Source/WebCore/html/FileInputType.cpp 2020-06-12 18:24:02 UTC (rev 262961)
+++ trunk/Source/WebCore/html/FileInputType.cpp 2020-06-12 18:40:25 UTC (rev 262962)
@@ -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()
{
- if (m_fileListCreator)
- m_fileListCreator->cancel();
-
if (m_fileChooser)
m_fileChooser->invalidate();
@@ -416,7 +413,10 @@
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));
Modified: trunk/Source/WebCore/html/FileInputType.h (262961 => 262962)
--- trunk/Source/WebCore/html/FileInputType.h 2020-06-12 18:24:02 UTC (rev 262961)
+++ trunk/Source/WebCore/html/FileInputType.h 2020-06-12 18:40:25 UTC (rev 262962)
@@ -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 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();
Modified: trunk/Source/WebCore/html/FileListCreator.cpp (262961 => 262962)
--- trunk/Source/WebCore/html/FileListCreator.cpp 2020-06-12 18:24:02 UTC (rev 262961)
+++ trunk/Source/WebCore/html/FileListCreator.cpp 2020-06-12 18:40:25 UTC (rev 262962)
@@ -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 @@
FileListCreator::~FileListCreator()
{
- ASSERT(!m_completionHander);
+ ASSERT(!m_completionHandler);
}
static void appendDirectoryFiles(const String& directory, const String& relativePath, Vector<Ref<File>>& fileObjects)
@@ -57,30 +57,12 @@
}
}
-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));
@@ -88,9 +70,33 @@
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;
}
Modified: trunk/Source/WebCore/html/FileListCreator.h (262961 => 262962)
--- trunk/Source/WebCore/html/FileListCreator.h 2020-06-12 18:24:02 UTC (rev 262961)
+++ trunk/Source/WebCore/html/FileListCreator.h 2020-06-12 18:40:25 UTC (rev 262962)
@@ -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,10 +41,7 @@
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();
@@ -51,13 +48,10 @@
void cancel();
private:
- FileListCreator(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories, CompletionHandler&&);
+ FileListCreator(const Vector<FileChooserFileInfo>&, CompletionHandler&&);
- template<ShouldResolveDirectories shouldResolveDirectories>
- static Ref<FileList> createFileList(const Vector<FileChooserFileInfo>&);
-
RefPtr<WorkQueue> m_workQueue;
- CompletionHandler m_completionHander;
+ CompletionHandler m_completionHandler;
};
}
