Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c481f5e

Browse filesBrowse files
aestesaestes
committed
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
1 parent 53f7c03 commit c481f5e
Copy full SHA for c481f5e

5 files changed

+77-40Lines changed: 77 additions & 40 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎Source/WebCore/ChangeLog‎

Copy file name to clipboardExpand all lines: Source/WebCore/ChangeLog
+36Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,39 @@
1+
2020-06-12 Andy Estes <aestes@apple.com>
2+
3+
FileInputType should use WeakPtr for FileListCreator lambdas
4+
https://bugs.webkit.org/show_bug.cgi?id=213130
5+
<rdar://problem/64276591>
6+
7+
Reviewed by David Kilzer.
8+
9+
FileInputType::filesChosen was passing a completion handler to FileListCreator::create that
10+
captured |this|. If the FileListCreator instance still existed when |this| was destroyed,
11+
FileInputType::~FileInputType would clear the captured |this| by calling
12+
FileListCreator::clear. This can be simplified by having the FileListCreator completion
13+
handler capture a WeakPtr to |this|.
14+
15+
Also, when FileInputType::allowsDirectories is false, m_fileListCreator would not be
16+
properly cleared after creating the file list. The FileListCreator completion handler would
17+
set m_fileListCreator to nullptr, but would be executed *before* FileListCreator::create
18+
returned and set m_fileListCreator to the newly-created FileListCreator object. Fixed this
19+
by having FileListCreator::create execute the completion handler immediately and return
20+
nullptr in cases where a FileListCreator does not need to be created for directory
21+
resolution.
22+
23+
Covered by existing tests.
24+
25+
* html/FileInputType.cpp:
26+
(WebCore::FileInputType::~FileInputType):
27+
(WebCore::FileInputType::filesChosen):
28+
* html/FileInputType.h:
29+
* html/FileListCreator.cpp:
30+
(WebCore::createFileList):
31+
(WebCore::FileListCreator::create):
32+
(WebCore::FileListCreator::FileListCreator):
33+
(WebCore::FileListCreator::createFileList):
34+
* html/FileListCreator.h:
35+
(WebCore::FileListCreator::create): Deleted.
36+
137
2020-06-12 Antti Koivisto <antti@apple.com>
238

339
REGRESSION (r262618): Very slow typing in a github issue
Collapse file

‎Source/WebCore/html/FileInputType.cpp‎

Copy file name to clipboardExpand all lines: Source/WebCore/html/FileInputType.cpp
+5-5Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2004-2018 Apple Inc. All rights reserved.
2+
* Copyright (C) 2004-2020 Apple Inc. All rights reserved.
33
* Copyright (C) 2010 Google Inc. All rights reserved.
44
*
55
* This library is free software; you can redistribute it and/or
@@ -103,9 +103,6 @@ FileInputType::FileInputType(HTMLInputElement& element)
103103

104104
FileInputType::~FileInputType()
105105
{
106-
if (m_fileListCreator)
107-
m_fileListCreator->cancel();
108-
109106
if (m_fileChooser)
110107
m_fileChooser->invalidate();
111108

@@ -416,7 +413,10 @@ void FileInputType::filesChosen(const Vector<FileChooserFileInfo>& paths, const
416413
m_fileListCreator->cancel();
417414

418415
auto shouldResolveDirectories = allowsDirectories() ? FileListCreator::ShouldResolveDirectories::Yes : FileListCreator::ShouldResolveDirectories::No;
419-
m_fileListCreator = FileListCreator::create(paths, shouldResolveDirectories, [this, icon = makeRefPtr(icon)](Ref<FileList>&& fileList) mutable {
416+
m_fileListCreator = FileListCreator::create(paths, shouldResolveDirectories, [this, weakThis = makeWeakPtr(*this), icon = makeRefPtr(icon)](Ref<FileList>&& fileList) mutable {
417+
auto protectedThis = makeRefPtr(weakThis.get());
418+
if (!protectedThis)
419+
return;
420420
setFiles(WTFMove(fileList), icon ? RequestIcon::Yes : RequestIcon::No);
421421
if (icon && !m_fileList->isEmpty() && element())
422422
iconLoaded(WTFMove(icon));
Collapse file

‎Source/WebCore/html/FileInputType.h‎

Copy file name to clipboardExpand all lines: Source/WebCore/html/FileInputType.h
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* Copyright (C) 2010 Google Inc. All rights reserved.
3-
* Copyright (C) 2011-2018 Apple Inc. All rights reserved.
3+
* Copyright (C) 2011-2020 Apple Inc. All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
66
* modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@
3535
#include "FileChooser.h"
3636
#include "FileIconLoader.h"
3737
#include <wtf/RefPtr.h>
38+
#include <wtf/WeakPtr.h>
3839

3940
namespace WebCore {
4041

@@ -43,7 +44,7 @@ class FileList;
4344
class FileListCreator;
4445
class Icon;
4546

46-
class FileInputType final : public BaseClickableWithKeyInputType, private FileChooserClient, private FileIconLoaderClient {
47+
class FileInputType final : public BaseClickableWithKeyInputType, private FileChooserClient, private FileIconLoaderClient, public CanMakeWeakPtr<FileInputType> {
4748
public:
4849
explicit FileInputType(HTMLInputElement&);
4950
virtual ~FileInputType();
Collapse file

‎Source/WebCore/html/FileListCreator.cpp‎

Copy file name to clipboardExpand all lines: Source/WebCore/html/FileListCreator.cpp
+29-23Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2017 Apple Inc. All rights reserved.
2+
* Copyright (C) 2017-2020 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -36,7 +36,7 @@ namespace WebCore {
3636

3737
FileListCreator::~FileListCreator()
3838
{
39-
ASSERT(!m_completionHander);
39+
ASSERT(!m_completionHandler);
4040
}
4141

4242
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
5757
}
5858
}
5959

60-
FileListCreator::FileListCreator(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
61-
{
62-
if (shouldResolveDirectories == ShouldResolveDirectories::No)
63-
completionHandler(createFileList<ShouldResolveDirectories::No>(paths));
64-
else {
65-
// Resolve directories on a background thread to avoid blocking the main thread.
66-
m_completionHander = WTFMove(completionHandler);
67-
m_workQueue = WorkQueue::create("FileListCreator Work Queue");
68-
m_workQueue->dispatch([this, protectedThis = makeRef(*this), paths = crossThreadCopy(paths)]() mutable {
69-
auto fileList = createFileList<ShouldResolveDirectories::Yes>(paths);
70-
callOnMainThread([this, protectedThis = WTFMove(protectedThis), fileList = WTFMove(fileList)]() mutable {
71-
if (auto completionHander = WTFMove(m_completionHander))
72-
completionHander(WTFMove(fileList));
73-
});
74-
});
75-
}
76-
}
77-
7860
template<FileListCreator::ShouldResolveDirectories shouldResolveDirectories>
79-
Ref<FileList> FileListCreator::createFileList(const Vector<FileChooserFileInfo>& paths)
61+
static Ref<FileList> createFileList(const Vector<FileChooserFileInfo>& paths)
8062
{
8163
Vector<Ref<File>> fileObjects;
8264
for (auto& info : paths) {
83-
if (shouldResolveDirectories == ShouldResolveDirectories::Yes && FileSystem::fileIsDirectory(info.path, FileSystem::ShouldFollowSymbolicLinks::No))
65+
if (shouldResolveDirectories == FileListCreator::ShouldResolveDirectories::Yes && FileSystem::fileIsDirectory(info.path, FileSystem::ShouldFollowSymbolicLinks::No))
8466
appendDirectoryFiles(info.path, FileSystem::pathGetFileName(info.path), fileObjects);
8567
else
8668
fileObjects.append(File::create(info.path, info.displayName));
8769
}
8870
return FileList::create(WTFMove(fileObjects));
8971
}
9072

73+
RefPtr<FileListCreator> FileListCreator::create(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
74+
{
75+
if (shouldResolveDirectories == ShouldResolveDirectories::No) {
76+
completionHandler(createFileList<ShouldResolveDirectories::No>(paths));
77+
return nullptr;
78+
}
79+
80+
return adoptRef(*new FileListCreator(paths, WTFMove(completionHandler)));
81+
}
82+
83+
FileListCreator::FileListCreator(const Vector<FileChooserFileInfo>& paths, CompletionHandler&& completionHandler)
84+
: m_workQueue(WorkQueue::create("FileListCreator Work Queue"))
85+
, m_completionHandler(WTFMove(completionHandler))
86+
{
87+
// Resolve directories on a background thread to avoid blocking the main thread.
88+
m_workQueue->dispatch([this, protectedThis = makeRef(*this), paths = crossThreadCopy(paths)]() mutable {
89+
auto fileList = createFileList<ShouldResolveDirectories::Yes>(paths);
90+
callOnMainThread([this, protectedThis = WTFMove(protectedThis), fileList = WTFMove(fileList)]() mutable {
91+
if (auto completionHandler = WTFMove(m_completionHandler))
92+
completionHandler(WTFMove(fileList));
93+
});
94+
});
95+
}
96+
9197
void FileListCreator::cancel()
9298
{
93-
m_completionHander = nullptr;
99+
m_completionHandler = nullptr;
94100
m_workQueue = nullptr;
95101
}
96102

Collapse file

‎Source/WebCore/html/FileListCreator.h‎

Copy file name to clipboardExpand all lines: Source/WebCore/html/FileListCreator.h
+4-10Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2017 Apple Inc. All rights reserved.
2+
* Copyright (C) 2017-2020 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -41,23 +41,17 @@ class FileListCreator : public ThreadSafeRefCounted<FileListCreator> {
4141
using CompletionHandler = Function<void(Ref<FileList>&&)>;
4242

4343
enum class ShouldResolveDirectories { No, Yes };
44-
static Ref<FileListCreator> create(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories shouldResolveDirectories, CompletionHandler&& completionHandler)
45-
{
46-
return adoptRef(*new FileListCreator(paths, shouldResolveDirectories, WTFMove(completionHandler)));
47-
}
44+
static RefPtr<FileListCreator> create(const Vector<FileChooserFileInfo>&, ShouldResolveDirectories, CompletionHandler&&);
4845

4946
~FileListCreator();
5047

5148
void cancel();
5249

5350
private:
54-
FileListCreator(const Vector<FileChooserFileInfo>& paths, ShouldResolveDirectories, CompletionHandler&&);
55-
56-
template<ShouldResolveDirectories shouldResolveDirectories>
57-
static Ref<FileList> createFileList(const Vector<FileChooserFileInfo>&);
51+
FileListCreator(const Vector<FileChooserFileInfo>&, CompletionHandler&&);
5852

5953
RefPtr<WorkQueue> m_workQueue;
60-
CompletionHandler m_completionHander;
54+
CompletionHandler m_completionHandler;
6155
};
6256

6357
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.