Log Message
Flaky Test: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker.html https://bugs.webkit.org/show_bug.cgi?id=182270 <rdar://problem/36904314>
Reviewed by Antti Koivisto. No new tests, already covered by existing tests that crash flakily on the bots. * loader/ThreadableLoaderClientWrapper.h: (WebCore::ThreadableLoaderClientWrapper::ThreadableLoaderClientWrapper): isolate copy the initiator string as this object can be destroyed on a different thread. This was causing the test to flakily crash as well when destroying ThreadLocalData. * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h: * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm: (scheduledWithCustomRunLoopMode): (-[WebCoreResourceHandleAsOperationQueueDelegate callFunctionOnMainThread:]): Fix thread safety issue in callFunctionOnMainThread. This function is called from a background thread to get to the main thread. However, it relied on m_handle which would get nullified on the main thread by detachHandle when the ResourceHandle is destroyed. Fix the issue by not relying on m_handle anymore. (-[WebCoreResourceHandleAsOperationQueueDelegate initWithHandle:messageQueue:]): (-[WebCoreResourceHandleAsOperationQueueDelegate connection:willSendRequest:redirectResponse:]): (-[WebCoreResourceHandleAsOperationQueueDelegate connection:canAuthenticateAgainstProtectionSpace:]): (-[WebCoreResourceHandleAsOperationQueueDelegate connection:didReceiveResponse:]): (-[WebCoreResourceHandleAsOperationQueueDelegate connection:willCacheResponse:]): - Go back to using autorelease() instead of get() for the returned objects to match the code pre-r224522. - Dispatch the protectedSelf variables that were added in r227073 to the main thread to make sure we do not get destroyed on the background thread when protectedSelf is the last strong reference to self. Destroying the WebCoreResourceHandleAsOperationQueueDelegate on the background safe is unsafe due to its m_messageQueue data member which contains lambdas that may capture anything. - Add a Lock to protect against detachHandle getting called on the main thread and nulling out m_handle / m_requestResult / m_cachedResponseResult while the background thread may be accessing them.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (228544 => 228545)
--- trunk/Source/WebCore/ChangeLog 2018-02-16 01:19:57 UTC (rev 228544)
+++ trunk/Source/WebCore/ChangeLog 2018-02-16 01:35:32 UTC (rev 228545)
@@ -1,3 +1,40 @@
+2018-02-15 Chris Dumez <[email protected]>
+
+ Flaky Test: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker.html
+ https://bugs.webkit.org/show_bug.cgi?id=182270
+ <rdar://problem/36904314>
+
+ Reviewed by Antti Koivisto.
+
+ No new tests, already covered by existing tests that crash flakily on the bots.
+
+ * loader/ThreadableLoaderClientWrapper.h:
+ (WebCore::ThreadableLoaderClientWrapper::ThreadableLoaderClientWrapper):
+ isolate copy the initiator string as this object can be destroyed on a different thread. This was
+ causing the test to flakily crash as well when destroying ThreadLocalData.
+
+ * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h:
+ * platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:
+ (scheduledWithCustomRunLoopMode):
+ (-[WebCoreResourceHandleAsOperationQueueDelegate callFunctionOnMainThread:]):
+ Fix thread safety issue in callFunctionOnMainThread. This function is called from a background thread
+ to get to the main thread. However, it relied on m_handle which would get nullified on the main thread
+ by detachHandle when the ResourceHandle is destroyed. Fix the issue by not relying on m_handle anymore.
+
+ (-[WebCoreResourceHandleAsOperationQueueDelegate initWithHandle:messageQueue:]):
+ (-[WebCoreResourceHandleAsOperationQueueDelegate connection:willSendRequest:redirectResponse:]):
+ (-[WebCoreResourceHandleAsOperationQueueDelegate connection:canAuthenticateAgainstProtectionSpace:]):
+ (-[WebCoreResourceHandleAsOperationQueueDelegate connection:didReceiveResponse:]):
+ (-[WebCoreResourceHandleAsOperationQueueDelegate connection:willCacheResponse:]):
+ - Go back to using autorelease() instead of get() for the returned objects to match the code pre-r224522.
+ - Dispatch the protectedSelf variables that were added in r227073 to the main thread to make sure we do
+ not get destroyed on the background thread when protectedSelf is the last strong reference to self.
+ Destroying the WebCoreResourceHandleAsOperationQueueDelegate on the background safe is unsafe due to
+ its m_messageQueue data member which contains lambdas that may capture anything.
+ - Add a Lock to protect against detachHandle getting called on the main thread and nulling out
+ m_handle / m_requestResult / m_cachedResponseResult while the background thread may be accessing
+ them.
+
2018-02-15 Zalan Bujtas <[email protected]>
[RenderTreeBuilder] Move RenderTableRow::addChild() to RenderTreeBuilder
Modified: trunk/Source/WebCore/loader/ThreadableLoaderClientWrapper.h (228544 => 228545)
--- trunk/Source/WebCore/loader/ThreadableLoaderClientWrapper.h 2018-02-16 01:19:57 UTC (rev 228544)
+++ trunk/Source/WebCore/loader/ThreadableLoaderClientWrapper.h 2018-02-16 01:35:32 UTC (rev 228545)
@@ -104,7 +104,7 @@
inline ThreadableLoaderClientWrapper::ThreadableLoaderClientWrapper(ThreadableLoaderClient& client, const String& initiator)
: m_client(&client)
- , m_initiator(initiator)
+ , m_initiator(initiator.isolatedCopy())
{
}
Modified: trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h (228544 => 228545)
--- trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h 2018-02-16 01:19:57 UTC (rev 228544)
+++ trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h 2018-02-16 01:35:32 UTC (rev 228545)
@@ -27,8 +27,10 @@
#include <dispatch/dispatch.h>
#include <wtf/Function.h>
+#include <wtf/Lock.h>
#include <wtf/MessageQueue.h>
#include <wtf/RetainPtr.h>
+#include <wtf/SchedulePair.h>
namespace WebCore {
class ResourceHandle;
@@ -41,7 +43,9 @@
dispatch_semaphore_t m_semaphore;
MessageQueue<Function<void()>>* m_messageQueue;
RetainPtr<NSURLRequest> m_requestResult;
+ Lock m_mutex;
RetainPtr<NSCachedURLResponse> m_cachedResponseResult;
+ std::optional<SchedulePairHashSet> m_scheduledPairs;
BOOL m_boolResult;
}
Modified: trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm (228544 => 228545)
--- trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm 2018-02-16 01:19:57 UTC (rev 228544)
+++ trunk/Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm 2018-02-16 01:35:32 UTC (rev 228545)
@@ -42,7 +42,7 @@
using namespace WebCore;
-static bool scheduledWithCustomRunLoopMode(SchedulePairHashSet* pairs)
+static bool scheduledWithCustomRunLoopMode(const std::optional<SchedulePairHashSet>& pairs)
{
if (!pairs)
return false;
@@ -63,8 +63,7 @@
return m_messageQueue->append(std::make_unique<Function<void()>>(WTFMove(function)));
// This is the common case.
- SchedulePairHashSet* pairs = m_handle && m_handle->context() ? m_handle->context()->scheduledRunLoopPairs() : nullptr;
- if (!scheduledWithCustomRunLoopMode(pairs))
+ if (!scheduledWithCustomRunLoopMode(m_scheduledPairs))
return callOnMainThread(WTFMove(function));
// If we have been scheduled in a custom run loop mode, schedule a block in that mode.
@@ -75,7 +74,7 @@
function();
function = nullptr;
});
- for (auto& pair : *pairs)
+ for (auto& pair : *m_scheduledPairs)
CFRunLoopPerformBlock(pair->runLoop(), pair->mode(), block.get());
}
@@ -86,6 +85,10 @@
return nil;
m_handle = handle;
+ if (m_handle && m_handle->context()) {
+ if (auto* pairs = m_handle->context()->scheduledRunLoopPairs())
+ m_scheduledPairs = *pairs;
+ }
m_semaphore = dispatch_semaphore_create(0);
m_messageQueue = messageQueue;
@@ -94,6 +97,8 @@
- (void)detachHandle
{
+ LockHolder lock(m_mutex);
+
m_handle = nullptr;
m_requestResult = nullptr;
@@ -139,7 +144,7 @@
#endif
auto protectedSelf = retainPtr(self);
- auto work = [self = self, protectedSelf = retainPtr(self), newRequest = retainPtr(newRequest), redirectResponse = retainPtr(redirectResponse)] () mutable {
+ auto work = [self, protectedSelf, newRequest = retainPtr(newRequest), redirectResponse = retainPtr(redirectResponse)] () mutable {
if (!m_handle) {
m_requestResult = nullptr;
dispatch_semaphore_signal(m_semaphore);
@@ -154,7 +159,18 @@
[self callFunctionOnMainThread:WTFMove(work)];
dispatch_semaphore_wait(m_semaphore, DISPATCH_TIME_FOREVER);
- return m_requestResult.get();
+
+ LockHolder lock(m_mutex);
+ if (!m_handle)
+ return nil;
+
+ RetainPtr<NSURLRequest> requestResult = m_requestResult;
+
+ // Make sure protectedSelf gets destroyed on the main thread in case this is the last strong reference to self
+ // as we do not want to get destroyed on a non-main thread.
+ [self callFunctionOnMainThread:[protectedSelf = WTFMove(protectedSelf)] { }];
+
+ return requestResult.autorelease();
}
- (void)connection:(NSURLConnection *)connection didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge
@@ -183,7 +199,7 @@
LOG(Network, "Handle %p delegate connection:%p canAuthenticateAgainstProtectionSpace:%@://%@:%u realm:%@ method:%@ %@%@", m_handle, connection, [protectionSpace protocol], [protectionSpace host], [protectionSpace port], [protectionSpace realm], [protectionSpace authenticationMethod], [protectionSpace isProxy] ? @"proxy:" : @"", [protectionSpace isProxy] ? [protectionSpace proxyType] : @"");
auto protectedSelf = retainPtr(self);
- auto work = [self = self, protectedSelf = retainPtr(self), protectionSpace = retainPtr(protectionSpace)] () mutable {
+ auto work = [self, protectedSelf, protectionSpace = retainPtr(protectionSpace)] () mutable {
if (!m_handle) {
m_boolResult = NO;
dispatch_semaphore_signal(m_semaphore);
@@ -194,7 +210,18 @@
[self callFunctionOnMainThread:WTFMove(work)];
dispatch_semaphore_wait(m_semaphore, DISPATCH_TIME_FOREVER);
- return m_boolResult;
+
+ LockHolder lock(m_mutex);
+ if (!m_handle)
+ return NO;
+
+ auto boolResult = m_boolResult;
+
+ // Make sure protectedSelf gets destroyed on the main thread in case this is the last strong reference to self
+ // as we do not want to get destroyed on a non-main thread.
+ [self callFunctionOnMainThread:[protectedSelf = WTFMove(protectedSelf)] { }];
+
+ return boolResult;
}
- (void)connection:(NSURLConnection *)connection didReceiveResponse:(NSURLResponse *)r
@@ -204,7 +231,7 @@
LOG(Network, "Handle %p delegate connection:%p didReceiveResponse:%p (HTTP status %d, reported MIMEType '%s')", m_handle, connection, r, [r respondsToSelector:@selector(statusCode)] ? [(id)r statusCode] : 0, [[r MIMEType] UTF8String]);
auto protectedSelf = retainPtr(self);
- auto work = [self = self, protectedSelf = retainPtr(self), r = retainPtr(r), connection = retainPtr(connection)] () mutable {
+ auto work = [self, protectedSelf, r = retainPtr(r), connection = retainPtr(connection)] () mutable {
RefPtr<ResourceHandle> protectedHandle(m_handle);
if (!m_handle || !m_handle->client()) {
dispatch_semaphore_signal(m_semaphore);
@@ -232,6 +259,9 @@
[self callFunctionOnMainThread:WTFMove(work)];
dispatch_semaphore_wait(m_semaphore, DISPATCH_TIME_FOREVER);
+
+ // Make sure we get destroyed on the main thread.
+ [self callFunctionOnMainThread:[protectedSelf = WTFMove(protectedSelf)] { }];
}
- (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data lengthReceived:(long long)lengthReceived
@@ -326,7 +356,7 @@
LOG(Network, "Handle %p delegate connection:%p willCacheResponse:%p", m_handle, connection, cachedResponse);
auto protectedSelf = retainPtr(self);
- auto work = [self = self, protectedSelf = retainPtr(self), cachedResponse = retainPtr(cachedResponse)] () mutable {
+ auto work = [self, protectedSelf, cachedResponse = retainPtr(cachedResponse)] () mutable {
if (!m_handle || !m_handle->client()) {
m_cachedResponseResult = nullptr;
dispatch_semaphore_signal(m_semaphore);
@@ -338,7 +368,18 @@
[self callFunctionOnMainThread:WTFMove(work)];
dispatch_semaphore_wait(m_semaphore, DISPATCH_TIME_FOREVER);
- return m_cachedResponseResult.get();
+
+ LockHolder lock(m_mutex);
+ if (!m_handle)
+ return nil;
+
+ RetainPtr<NSCachedURLResponse> cachedResponseResult = m_cachedResponseResult;
+
+ // Make sure protectedSelf gets destroyed on the main thread in case this is the last strong reference to self
+ // as we do not want to get destroyed on a non-main thread.
+ [self callFunctionOnMainThread:[protectedSelf = WTFMove(protectedSelf)] { }];
+
+ return cachedResponseResult.autorelease();
}
@end
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
