Make WKURLSchemeTask thread safe.
<rdar://problem/50471863> and https://bugs.webkit.org/show_bug.cgi?id=199764
Reviewed by Alex Christensen.
Source/WebKit:
Punt most of the WKURLSchemeTask operations back to the main thread.
Make accessing the NSURLRequest be thread safe with lock protection.
* UIProcess/API/Cocoa/WKURLSchemeTask.mm:
(getExceptionTypeFromMainRunLoop):
(-[WKURLSchemeTaskImpl dealloc]):
(-[WKURLSchemeTaskImpl request]):
(-[WKURLSchemeTaskImpl _requestOnlyIfCached]):
(-[WKURLSchemeTaskImpl didReceiveResponse:]):
(-[WKURLSchemeTaskImpl didReceiveData:]):
(-[WKURLSchemeTaskImpl didFinish]):
(-[WKURLSchemeTaskImpl didFailWithError:]):
(-[WKURLSchemeTaskImpl _didPerformRedirection:newRequest:]):
* UIProcess/WebURLSchemeTask.cpp:
(WebKit::WebURLSchemeTask::WebURLSchemeTask):
(WebKit::WebURLSchemeTask::~WebURLSchemeTask):
(WebKit::WebURLSchemeTask::didPerformRedirection):
(WebKit::WebURLSchemeTask::didReceiveResponse):
(WebKit::WebURLSchemeTask::didReceiveData):
(WebKit::WebURLSchemeTask::didComplete):
(WebKit::WebURLSchemeTask::pageDestroyed):
(WebKit::WebURLSchemeTask::stop):
(WebKit::WebURLSchemeTask::nsRequest const):
* UIProcess/WebURLSchemeTask.h:
(WebKit::WebURLSchemeTask::identifier const):
(WebKit::WebURLSchemeTask::pageID const):
(WebKit::WebURLSchemeTask::process):
(WebKit::WebURLSchemeTask::process const): Deleted.
(WebKit::WebURLSchemeTask::request const): Deleted.
Source/WTF:
* wtf/MainThread.cpp:
(WTF::callOnMainAndWait):
(WTF::callOnMainRunLoopAndWait):
(WTF::callOnMainThreadAndWait):
* wtf/MainThread.h:
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@247461 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
index 2e4a5ee..b902892 100644
--- a/Source/WTF/ChangeLog
+++ b/Source/WTF/ChangeLog
@@ -1,3 +1,16 @@
+2019-07-15 Brady Eidson <beidson@apple.com>
+
+ Make WKURLSchemeTask thread safe.
+ <rdar://problem/50471863> and https://bugs.webkit.org/show_bug.cgi?id=199764
+
+ Reviewed by Alex Christensen.
+
+ * wtf/MainThread.cpp:
+ (WTF::callOnMainAndWait):
+ (WTF::callOnMainRunLoopAndWait):
+ (WTF::callOnMainThreadAndWait):
+ * wtf/MainThread.h:
+
2019-07-15 Dean Jackson <dino@apple.com>
MacCatalyst asserts when command key is raised
diff --git a/Source/WTF/wtf/MainThread.cpp b/Source/WTF/wtf/MainThread.cpp
index 9edd8ef..e2ae1d4 100644
--- a/Source/WTF/wtf/MainThread.cpp
+++ b/Source/WTF/wtf/MainThread.cpp
@@ -174,9 +174,15 @@
return isMainThread();
}
-void callOnMainThreadAndWait(WTF::Function<void()>&& function)
+enum class MainStyle : bool {
+ Thread,
+ RunLoop
+};
+
+static void callOnMainAndWait(WTF::Function<void()>&& function, MainStyle mainStyle)
{
- if (isMainThread()) {
+
+ if (mainStyle == MainStyle::Thread ? isMainThread() : isMainRunLoop()) {
function();
return;
}
@@ -186,13 +192,21 @@
bool isFinished = false;
- callOnMainThread([&, function = WTFMove(function)] {
+ auto functionImpl = [&, function = WTFMove(function)] {
function();
std::lock_guard<Lock> lock(mutex);
isFinished = true;
conditionVariable.notifyOne();
- });
+ };
+
+ switch (mainStyle) {
+ case MainStyle::Thread:
+ callOnMainThread(WTFMove(functionImpl));
+ break;
+ case MainStyle::RunLoop:
+ callOnMainRunLoop(WTFMove(functionImpl));
+ };
std::unique_lock<Lock> lock(mutex);
conditionVariable.wait(lock, [&] {
@@ -200,4 +214,14 @@
});
}
+void callOnMainRunLoopAndWait(WTF::Function<void()>&& function)
+{
+ callOnMainAndWait(WTFMove(function), MainStyle::RunLoop);
+}
+
+void callOnMainThreadAndWait(WTF::Function<void()>&& function)
+{
+ callOnMainAndWait(WTFMove(function), MainStyle::Thread);
+}
+
} // namespace WTF
diff --git a/Source/WTF/wtf/MainThread.h b/Source/WTF/wtf/MainThread.h
index 0834133..12bfae4 100644
--- a/Source/WTF/wtf/MainThread.h
+++ b/Source/WTF/wtf/MainThread.h
@@ -59,6 +59,7 @@
WTF_EXPORT_PRIVATE bool isMainRunLoop();
WTF_EXPORT_PRIVATE void callOnMainRunLoop(Function<void()>&&);
+WTF_EXPORT_PRIVATE void callOnMainRunLoopAndWait(Function<void()>&&);
#if USE(WEB_THREAD)
WTF_EXPORT_PRIVATE bool isWebThread();
@@ -92,6 +93,8 @@
using WTF::callOnMainThread;
using WTF::callOnMainThreadAndWait;
+using WTF::callOnMainRunLoop;
+using WTF::callOnMainRunLoopAndWait;
using WTF::canAccessThreadLocalDataForThread;
using WTF::isMainThread;
using WTF::isMainThreadOrGCThread;
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 77ed6be..cfd15bc 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,42 @@
+2019-07-15 Brady Eidson <beidson@apple.com>
+
+ Make WKURLSchemeTask thread safe.
+ <rdar://problem/50471863> and https://bugs.webkit.org/show_bug.cgi?id=199764
+
+ Reviewed by Alex Christensen.
+
+ Punt most of the WKURLSchemeTask operations back to the main thread.
+ Make accessing the NSURLRequest be thread safe with lock protection.
+
+ * UIProcess/API/Cocoa/WKURLSchemeTask.mm:
+ (getExceptionTypeFromMainRunLoop):
+ (-[WKURLSchemeTaskImpl dealloc]):
+ (-[WKURLSchemeTaskImpl request]):
+ (-[WKURLSchemeTaskImpl _requestOnlyIfCached]):
+ (-[WKURLSchemeTaskImpl didReceiveResponse:]):
+ (-[WKURLSchemeTaskImpl didReceiveData:]):
+ (-[WKURLSchemeTaskImpl didFinish]):
+ (-[WKURLSchemeTaskImpl didFailWithError:]):
+ (-[WKURLSchemeTaskImpl _didPerformRedirection:newRequest:]):
+
+ * UIProcess/WebURLSchemeTask.cpp:
+ (WebKit::WebURLSchemeTask::WebURLSchemeTask):
+ (WebKit::WebURLSchemeTask::~WebURLSchemeTask):
+ (WebKit::WebURLSchemeTask::didPerformRedirection):
+ (WebKit::WebURLSchemeTask::didReceiveResponse):
+ (WebKit::WebURLSchemeTask::didReceiveData):
+ (WebKit::WebURLSchemeTask::didComplete):
+ (WebKit::WebURLSchemeTask::pageDestroyed):
+ (WebKit::WebURLSchemeTask::stop):
+ (WebKit::WebURLSchemeTask::nsRequest const):
+
+ * UIProcess/WebURLSchemeTask.h:
+ (WebKit::WebURLSchemeTask::identifier const):
+ (WebKit::WebURLSchemeTask::pageID const):
+ (WebKit::WebURLSchemeTask::process):
+ (WebKit::WebURLSchemeTask::process const): Deleted.
+ (WebKit::WebURLSchemeTask::request const): Deleted.
+
2019-07-15 Wenson Hsieh <wenson_hsieh@apple.com>
Followup to r247439
diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm b/Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm
index 3db32e3..ceb10ce 100644
--- a/Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm
+++ b/Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -32,6 +32,20 @@
#import <WebCore/ResourceResponse.h>
#import <WebCore/SharedBuffer.h>
#import <wtf/BlockPtr.h>
+#import <wtf/MainThread.h>
+
+static WebKit::WebURLSchemeTask::ExceptionType getExceptionTypeFromMainRunLoop(Function<WebKit::WebURLSchemeTask::ExceptionType ()>&& function)
+{
+ if (RunLoop::isMain())
+ return function();
+
+ WebKit::WebURLSchemeTask::ExceptionType exceptionType;
+ callOnMainRunLoopAndWait([function = WTFMove(function), &exceptionType] {
+ exceptionType = function();
+ });
+
+ return exceptionType;
+}
static void raiseExceptionIfNecessary(WebKit::WebURLSchemeTask::ExceptionType exceptionType)
{
@@ -60,48 +74,75 @@
- (void)dealloc
{
- _urlSchemeTask->API::URLSchemeTask::~URLSchemeTask();
+ auto func = [task = WTFMove(_urlSchemeTask)] () mutable {
+ task->API::URLSchemeTask::~URLSchemeTask();
+ };
+
+ if (RunLoop::isMain())
+ func();
+ else
+ callOnMainRunLoop(WTFMove(func));
[super dealloc];
}
- (NSURLRequest *)request
{
- return _urlSchemeTask->task().request().nsURLRequest(WebCore::HTTPBodyUpdatePolicy::UpdateHTTPBody);
+ return _urlSchemeTask->task().nsRequest();
}
- (BOOL)_requestOnlyIfCached
{
- return _urlSchemeTask->task().request().cachePolicy() == WebCore::ResourceRequestCachePolicy::ReturnCacheDataDontLoad;
+ return _urlSchemeTask->task().nsRequest().cachePolicy == NSURLRequestReturnCacheDataDontLoad;
}
- (void)didReceiveResponse:(NSURLResponse *)response
{
- auto result = _urlSchemeTask->task().didReceiveResponse(response);
+ auto function = [protectedSelf = retainPtr(self), self, protectedResponse = retainPtr(response), response] {
+ return _urlSchemeTask->task().didReceiveResponse(response);
+ };
+
+ auto result = getExceptionTypeFromMainRunLoop(WTFMove(function));
raiseExceptionIfNecessary(result);
}
- (void)didReceiveData:(NSData *)data
{
- auto result = _urlSchemeTask->task().didReceiveData(WebCore::SharedBuffer::create(data));
+ auto function = [protectedSelf = retainPtr(self), self, protectedData = retainPtr(data), data] () mutable {
+ return _urlSchemeTask->task().didReceiveData(WebCore::SharedBuffer::create(data));
+ };
+
+ auto result = getExceptionTypeFromMainRunLoop(WTFMove(function));
raiseExceptionIfNecessary(result);
}
- (void)didFinish
{
- auto result = _urlSchemeTask->task().didComplete({ });
+ auto function = [protectedSelf = retainPtr(self), self] {
+ return _urlSchemeTask->task().didComplete({ });
+ };
+
+ auto result = getExceptionTypeFromMainRunLoop(WTFMove(function));
raiseExceptionIfNecessary(result);
}
- (void)didFailWithError:(NSError *)error
{
- auto result = _urlSchemeTask->task().didComplete(error);
+ auto function = [protectedSelf = retainPtr(self), self, protectedError = retainPtr(error), error] {
+ return _urlSchemeTask->task().didComplete(error);
+ };
+
+ auto result = getExceptionTypeFromMainRunLoop(WTFMove(function));
raiseExceptionIfNecessary(result);
}
- (void)_didPerformRedirection:(NSURLResponse *)response newRequest:(NSURLRequest *)request
{
- auto result = _urlSchemeTask->task().didPerformRedirection(response, request);
+ auto function = [protectedSelf = retainPtr(self), self, protectedResponse = retainPtr(response), response, protectedRequest = retainPtr(request), request] {
+ return _urlSchemeTask->task().didPerformRedirection(response, request);
+ };
+
+ auto result = getExceptionTypeFromMainRunLoop(WTFMove(function));
raiseExceptionIfNecessary(result);
}
diff --git a/Source/WebKit/UIProcess/WebURLSchemeTask.cpp b/Source/WebKit/UIProcess/WebURLSchemeTask.cpp
index 24866e1..03621c0 100644
--- a/Source/WebKit/UIProcess/WebURLSchemeTask.cpp
+++ b/Source/WebKit/UIProcess/WebURLSchemeTask.cpp
@@ -50,10 +50,18 @@
, m_request(WTFMove(request))
, m_syncCompletionHandler(WTFMove(syncCompletionHandler))
{
+ ASSERT(RunLoop::isMain());
+}
+
+WebURLSchemeTask::~WebURLSchemeTask()
+{
+ ASSERT(RunLoop::isMain());
}
auto WebURLSchemeTask::didPerformRedirection(WebCore::ResourceResponse&& response, WebCore::ResourceRequest&& request) -> ExceptionType
{
+ ASSERT(RunLoop::isMain());
+
if (m_stopped)
return ExceptionType::TaskAlreadyStopped;
@@ -69,7 +77,11 @@
if (isSync())
m_syncResponse = response;
- m_request = request;
+ {
+ LockHolder locker(m_requestLock);
+ m_request = request;
+ }
+
m_process->send(Messages::WebPage::URLSchemeTaskDidPerformRedirection(m_urlSchemeHandler->identifier(), m_identifier, response, request), m_page->pageID());
return ExceptionType::None;
@@ -77,6 +89,8 @@
auto WebURLSchemeTask::didReceiveResponse(const ResourceResponse& response) -> ExceptionType
{
+ ASSERT(RunLoop::isMain());
+
if (m_stopped)
return ExceptionType::TaskAlreadyStopped;
@@ -99,6 +113,8 @@
auto WebURLSchemeTask::didReceiveData(Ref<SharedBuffer>&& buffer) -> ExceptionType
{
+ ASSERT(RunLoop::isMain());
+
if (m_stopped)
return ExceptionType::TaskAlreadyStopped;
@@ -124,6 +140,8 @@
auto WebURLSchemeTask::didComplete(const ResourceError& error) -> ExceptionType
{
+ ASSERT(RunLoop::isMain());
+
if (m_stopped)
return ExceptionType::TaskAlreadyStopped;
@@ -154,22 +172,38 @@
void WebURLSchemeTask::pageDestroyed()
{
+ ASSERT(RunLoop::isMain());
ASSERT(m_page);
+
m_page = nullptr;
m_process = nullptr;
m_stopped = true;
- if (isSync())
+ if (isSync()) {
+ LockHolder locker(m_requestLock);
m_syncCompletionHandler({ }, failedCustomProtocolSyncLoad(m_request), { });
+ }
}
void WebURLSchemeTask::stop()
{
+ ASSERT(RunLoop::isMain());
ASSERT(!m_stopped);
+
m_stopped = true;
- if (isSync())
+ if (isSync()) {
+ LockHolder locker(m_requestLock);
m_syncCompletionHandler({ }, failedCustomProtocolSyncLoad(m_request), { });
+ }
}
+#if PLATFORM(COCOA)
+NSURLRequest *WebURLSchemeTask::nsRequest() const
+{
+ LockHolder locker(m_requestLock);
+ return m_request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::UpdateHTTPBody);
+}
+#endif
+
} // namespace WebKit
diff --git a/Source/WebKit/UIProcess/WebURLSchemeTask.h b/Source/WebKit/UIProcess/WebURLSchemeTask.h
index aec8645..7012b98 100644
--- a/Source/WebKit/UIProcess/WebURLSchemeTask.h
+++ b/Source/WebKit/UIProcess/WebURLSchemeTask.h
@@ -51,16 +51,21 @@
using SyncLoadCompletionHandler = CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const Vector<char>&)>;
-class WebURLSchemeTask : public RefCounted<WebURLSchemeTask>, public InstanceCounted<WebURLSchemeTask> {
+class WebURLSchemeTask : public ThreadSafeRefCounted<WebURLSchemeTask>, public InstanceCounted<WebURLSchemeTask> {
WTF_MAKE_NONCOPYABLE(WebURLSchemeTask);
public:
static Ref<WebURLSchemeTask> create(WebURLSchemeHandler&, WebPageProxy&, WebProcessProxy&, uint64_t identifier, WebCore::ResourceRequest&&, SyncLoadCompletionHandler&&);
- uint64_t identifier() const { return m_identifier; }
- WebCore::PageIdentifier pageID() const { return m_pageIdentifier; }
- WebProcessProxy* process() const { return m_process.get(); }
+ ~WebURLSchemeTask();
- const WebCore::ResourceRequest& request() const { return m_request; }
+ uint64_t identifier() const { ASSERT(RunLoop::isMain()); return m_identifier; }
+ WebCore::PageIdentifier pageID() const { ASSERT(RunLoop::isMain()); return m_pageIdentifier; }
+
+ WebProcessProxy* process() { ASSERT(RunLoop::isMain()); return m_process.get(); }
+
+#if PLATFORM(COCOA)
+ NSURLRequest *nsRequest() const;
+#endif
enum class ExceptionType {
DataAlreadySent,
@@ -89,6 +94,7 @@
uint64_t m_identifier;
WebCore::PageIdentifier m_pageIdentifier;
WebCore::ResourceRequest m_request;
+ mutable Lock m_requestLock;
bool m_stopped { false };
bool m_responseSent { false };
bool m_dataSent { false };
diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 3af19e1..8f42aac 100644
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,12 @@
+2019-07-15 Brady Eidson <beidson@apple.com>
+
+ Make WKURLSchemeTask thread safe.
+ <rdar://problem/50471863> and https://bugs.webkit.org/show_bug.cgi?id=199764
+
+ Reviewed by Alex Christensen.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:
+
2019-07-15 Jiewen Tan <jiewen_tan@apple.com>
Unreviewed, a build fix after r247437
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm
index f34eb3b..a64e007 100644
--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm
+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm
@@ -34,8 +34,11 @@
#import <WebKit/WKURLSchemeTaskPrivate.h>
#import <WebKit/WKWebViewConfigurationPrivate.h>
#import <WebKit/WebKit.h>
+#import <wtf/BlockPtr.h>
#import <wtf/HashMap.h>
#import <wtf/RetainPtr.h>
+#import <wtf/RunLoop.h>
+#import <wtf/Threading.h>
#import <wtf/Vector.h>
#import <wtf/text/StringHash.h>
#import <wtf/text/WTFString.h>
@@ -692,3 +695,41 @@
TestWebKitAPI::Util::run(&done);
}
+TEST(URLSchemeHandler, Threads)
+{
+ NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
+
+ auto handler = adoptNS([[TestURLSchemeHandler alloc] init]);
+ auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [configuration setURLSchemeHandler:handler.get() forURLScheme:@"threads"];
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration.get()]);
+
+ static bool done;
+ static id<WKURLSchemeTask> theTask;
+ static RefPtr<Thread> theThread;
+ [handler setStartURLSchemeTaskHandler:^(WKWebView *, id<WKURLSchemeTask> task) {
+ theTask = task;
+ [task retain];
+ theThread = Thread::create("A", [task] {
+ auto response = adoptNS([[NSURLResponse alloc] initWithURL:task.request.URL MIMEType:@"text/html" expectedContentLength:0 textEncodingName:nil]);
+ [task didReceiveResponse:response.get()];
+ [task didFinish];
+ done = true;
+ });
+ }];
+
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"threads://main.html"]]];
+
+ TestWebKitAPI::Util::run(&done);
+
+ handler = nil;
+ configuration = nil;
+ webView = nil;
+ theThread = nullptr;
+ [pool drain];
+
+ Thread::create("B", [] {
+ [theTask release];
+ theTask = nil;
+ })->waitForCompletion();
+}
