Title: [228903] trunk/Source/WebCore
- Revision
- 228903
- Author
- [email protected]
- Date
- 2018-02-21 15:56:16 -0800 (Wed, 21 Feb 2018)
Log Message
Regression(r228708): Crash under WebCore::MediaResource::responseReceived(WebCore::CachedResource&, WebCore::ResourceResponse const&)
https://bugs.webkit.org/show_bug.cgi?id=183018
<rdar://problem/37754154>
Reviewed by Eric Carlson.
The fix at r228708 was trying to address the fact that avplayer sometimes
deallocates WebCoreNSURLSessionDataTask objects on a non-main thread, which
was not safe because its _resource data member needs to be deallocated on
the main thread.
The issue is that r228708 caused _resource to outlive its WebCoreNSURLSessionDataTask.
This is an issue because _resource has a client data member (of type WebCoreNSURLSessionDataTaskClient)
which has a raw pointer to the WebCoreNSURLSessionDataTask. This means that the main thread could
call methods like responseReceived() on the resource, which would call responseReceived() on the
client, which would try to call [WebCoreNSURLSessionDataTask receivedResponse:] with an invalid
m_task pointer.
To address the issue, I introduced a clearTask() method on WebCoreNSURLSessionDataTaskClient, which
gets called from a non-main thread to clear the client's m_task pointer when the task is destroyed
on a non-main thread. So that this is safe, every time the client tries to use m_task, we now
acquire a lock for thread-safety and do a null-check on m_task.
No new tests, no known reproduction case.
* platform/graphics/PlatformMediaResourceLoader.h:
(WebCore::PlatformMediaResource::client):
* platform/network/cocoa/WebCoreNSURLSession.mm:
(WebCore::WebCoreNSURLSessionDataTaskClient::clearTask):
(WebCore::WebCoreNSURLSessionDataTaskClient::dataSent):
(WebCore::WebCoreNSURLSessionDataTaskClient::responseReceived):
(WebCore::WebCoreNSURLSessionDataTaskClient::shouldCacheResponse):
(WebCore::WebCoreNSURLSessionDataTaskClient::dataReceived):
(WebCore::WebCoreNSURLSessionDataTaskClient::redirectReceived):
(WebCore::WebCoreNSURLSessionDataTaskClient::accessControlCheckFailed):
(WebCore::WebCoreNSURLSessionDataTaskClient::loadFailed):
(WebCore::WebCoreNSURLSessionDataTaskClient::loadFinished):
(-[WebCoreNSURLSessionDataTask dealloc]):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (228902 => 228903)
--- trunk/Source/WebCore/ChangeLog 2018-02-21 23:31:15 UTC (rev 228902)
+++ trunk/Source/WebCore/ChangeLog 2018-02-21 23:56:16 UTC (rev 228903)
@@ -1,3 +1,44 @@
+2018-02-21 Chris Dumez <[email protected]>
+
+ Regression(r228708): Crash under WebCore::MediaResource::responseReceived(WebCore::CachedResource&, WebCore::ResourceResponse const&)
+ https://bugs.webkit.org/show_bug.cgi?id=183018
+ <rdar://problem/37754154>
+
+ Reviewed by Eric Carlson.
+
+ The fix at r228708 was trying to address the fact that avplayer sometimes
+ deallocates WebCoreNSURLSessionDataTask objects on a non-main thread, which
+ was not safe because its _resource data member needs to be deallocated on
+ the main thread.
+
+ The issue is that r228708 caused _resource to outlive its WebCoreNSURLSessionDataTask.
+ This is an issue because _resource has a client data member (of type WebCoreNSURLSessionDataTaskClient)
+ which has a raw pointer to the WebCoreNSURLSessionDataTask. This means that the main thread could
+ call methods like responseReceived() on the resource, which would call responseReceived() on the
+ client, which would try to call [WebCoreNSURLSessionDataTask receivedResponse:] with an invalid
+ m_task pointer.
+
+ To address the issue, I introduced a clearTask() method on WebCoreNSURLSessionDataTaskClient, which
+ gets called from a non-main thread to clear the client's m_task pointer when the task is destroyed
+ on a non-main thread. So that this is safe, every time the client tries to use m_task, we now
+ acquire a lock for thread-safety and do a null-check on m_task.
+
+ No new tests, no known reproduction case.
+
+ * platform/graphics/PlatformMediaResourceLoader.h:
+ (WebCore::PlatformMediaResource::client):
+ * platform/network/cocoa/WebCoreNSURLSession.mm:
+ (WebCore::WebCoreNSURLSessionDataTaskClient::clearTask):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::dataSent):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::responseReceived):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::shouldCacheResponse):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::dataReceived):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::redirectReceived):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::accessControlCheckFailed):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::loadFailed):
+ (WebCore::WebCoreNSURLSessionDataTaskClient::loadFinished):
+ (-[WebCoreNSURLSessionDataTask dealloc]):
+
2018-02-21 Youenn Fablet <[email protected]>
Move AppCache loading to the NetworkProcess
Modified: trunk/Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h (228902 => 228903)
--- trunk/Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h 2018-02-21 23:31:15 UTC (rev 228902)
+++ trunk/Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h 2018-02-21 23:56:16 UTC (rev 228903)
@@ -80,6 +80,7 @@
virtual bool didPassAccessControlCheck() const { return false; }
void setClient(std::unique_ptr<PlatformMediaResourceClient>&& client) { m_client = WTFMove(client); }
+ PlatformMediaResourceClient* client() { return m_client.get(); }
protected:
std::unique_ptr<PlatformMediaResourceClient> m_client;
Modified: trunk/Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm (228902 => 228903)
--- trunk/Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm 2018-02-21 23:31:15 UTC (rev 228902)
+++ trunk/Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm 2018-02-21 23:56:16 UTC (rev 228903)
@@ -354,6 +354,8 @@
{
}
+ void clearTask();
+
void responseReceived(PlatformMediaResource&, const ResourceResponse&) override;
void redirectReceived(PlatformMediaResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) override;
bool shouldCacheResponse(PlatformMediaResource&, const ResourceResponse&) override;
@@ -364,31 +366,58 @@
void loadFinished(PlatformMediaResource&) override;
private:
+ Lock m_taskLock;
WebCoreNSURLSessionDataTask *m_task;
};
+void WebCoreNSURLSessionDataTaskClient::clearTask()
+{
+ LockHolder locker(m_taskLock);
+ m_task = nullptr;
+}
+
void WebCoreNSURLSessionDataTaskClient::dataSent(PlatformMediaResource& resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource sentBytes:bytesSent totalBytesToBeSent:totalBytesToBeSent];
}
void WebCoreNSURLSessionDataTaskClient::responseReceived(PlatformMediaResource& resource, const ResourceResponse& response)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource receivedResponse:response];
}
bool WebCoreNSURLSessionDataTaskClient::shouldCacheResponse(PlatformMediaResource& resource, const ResourceResponse& response)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return false;
+
return [m_task resource:resource shouldCacheResponse:response];
}
void WebCoreNSURLSessionDataTaskClient::dataReceived(PlatformMediaResource& resource, const char* data, int length)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource receivedData:data length:length];
}
void WebCoreNSURLSessionDataTaskClient::redirectReceived(PlatformMediaResource& resource, ResourceRequest&& request, const ResourceResponse& response, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource receivedRedirect:response request:WTFMove(request) completionHandler: [completionHandler = WTFMove(completionHandler)] (auto&& request) {
ASSERT(isMainThread());
completionHandler(WTFMove(request));
@@ -397,16 +426,28 @@
void WebCoreNSURLSessionDataTaskClient::accessControlCheckFailed(PlatformMediaResource& resource, const ResourceError& error)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource accessControlCheckFailedWithError:error];
}
void WebCoreNSURLSessionDataTaskClient::loadFailed(PlatformMediaResource& resource, const ResourceError& error)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resource:resource loadFailedWithError:error];
}
void WebCoreNSURLSessionDataTaskClient::loadFinished(PlatformMediaResource& resource)
{
+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
[m_task resourceFinished:resource];
}
@@ -539,7 +580,13 @@
[_currentRequest release];
[_error release];
[_taskDescription release];
- callOnMainThread([resource = WTFMove(_resource)] { });
+
+ if (!isMainThread() && _resource) {
+ if (auto* client = _resource->client())
+ static_cast<WebCoreNSURLSessionDataTaskClient*>(client)->clearTask();
+ callOnMainThread([resource = WTFMove(_resource)] { });
+ }
+
[super dealloc];
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
