<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[228903] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/228903">228903</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2018-02-21 15:56:16 -0800 (Wed, 21 Feb 2018)</dd>
</dl>
<h3>Log Message</h3>
<pre>Regression(<a href="http://trac.webkit.org/projects/webkit/changeset/228708">r228708</a>): 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 <a href="http://trac.webkit.org/projects/webkit/changeset/228708">r228708</a> 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 <a href="http://trac.webkit.org/projects/webkit/changeset/228708">r228708</a> 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]):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreplatformgraphicsPlatformMediaResourceLoaderh">trunk/Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h</a></li>
<li><a href="#trunkSourceWebCoreplatformnetworkcocoaWebCoreNSURLSessionmm">trunk/Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (228902 => 228903)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,44 @@
</span><ins>+2018-02-21 Chris Dumez <cdumez@apple.com>
+
+ 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]):
+
</ins><span class="cx"> 2018-02-21 Youenn Fablet <youenn@apple.com>
</span><span class="cx">
</span><span class="cx"> Move AppCache loading to the NetworkProcess
</span></span></pre></div>
<a id="trunkSourceWebCoreplatformgraphicsPlatformMediaResourceLoaderh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h (228902 => 228903)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -80,6 +80,7 @@
</span><span class="cx"> virtual bool didPassAccessControlCheck() const { return false; }
</span><span class="cx">
</span><span class="cx"> void setClient(std::unique_ptr<PlatformMediaResourceClient>&& client) { m_client = WTFMove(client); }
</span><ins>+ PlatformMediaResourceClient* client() { return m_client.get(); }
</ins><span class="cx">
</span><span class="cx"> protected:
</span><span class="cx"> std::unique_ptr<PlatformMediaResourceClient> m_client;
</span></span></pre></div>
<a id="trunkSourceWebCoreplatformnetworkcocoaWebCoreNSURLSessionmm"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm (228902 => 228903)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -354,6 +354,8 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ void clearTask();
+
</ins><span class="cx"> void responseReceived(PlatformMediaResource&, const ResourceResponse&) override;
</span><span class="cx"> void redirectReceived(PlatformMediaResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) override;
</span><span class="cx"> bool shouldCacheResponse(PlatformMediaResource&, const ResourceResponse&) override;
</span><span class="lines">@@ -364,31 +366,58 @@
</span><span class="cx"> void loadFinished(PlatformMediaResource&) override;
</span><span class="cx">
</span><span class="cx"> private:
</span><ins>+ Lock m_taskLock;
</ins><span class="cx"> WebCoreNSURLSessionDataTask *m_task;
</span><span class="cx"> };
</span><span class="cx">
</span><ins>+void WebCoreNSURLSessionDataTaskClient::clearTask()
+{
+ LockHolder locker(m_taskLock);
+ m_task = nullptr;
+}
+
</ins><span class="cx"> void WebCoreNSURLSessionDataTaskClient::dataSent(PlatformMediaResource& resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource sentBytes:bytesSent totalBytesToBeSent:totalBytesToBeSent];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::responseReceived(PlatformMediaResource& resource, const ResourceResponse& response)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource receivedResponse:response];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool WebCoreNSURLSessionDataTaskClient::shouldCacheResponse(PlatformMediaResource& resource, const ResourceResponse& response)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return false;
+
</ins><span class="cx"> return [m_task resource:resource shouldCacheResponse:response];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::dataReceived(PlatformMediaResource& resource, const char* data, int length)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource receivedData:data length:length];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::redirectReceived(PlatformMediaResource& resource, ResourceRequest&& request, const ResourceResponse& response, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource receivedRedirect:response request:WTFMove(request) completionHandler: [completionHandler = WTFMove(completionHandler)] (auto&& request) {
</span><span class="cx"> ASSERT(isMainThread());
</span><span class="cx"> completionHandler(WTFMove(request));
</span><span class="lines">@@ -397,16 +426,28 @@
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::accessControlCheckFailed(PlatformMediaResource& resource, const ResourceError& error)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource accessControlCheckFailedWithError:error];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::loadFailed(PlatformMediaResource& resource, const ResourceError& error)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resource:resource loadFailedWithError:error];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void WebCoreNSURLSessionDataTaskClient::loadFinished(PlatformMediaResource& resource)
</span><span class="cx"> {
</span><ins>+ LockHolder locker(m_taskLock);
+ if (!m_task)
+ return;
+
</ins><span class="cx"> [m_task resourceFinished:resource];
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -539,7 +580,13 @@
</span><span class="cx"> [_currentRequest release];
</span><span class="cx"> [_error release];
</span><span class="cx"> [_taskDescription release];
</span><del>- callOnMainThread([resource = WTFMove(_resource)] { });
</del><ins>+
+ if (!isMainThread() && _resource) {
+ if (auto* client = _resource->client())
+ static_cast<WebCoreNSURLSessionDataTaskClient*>(client)->clearTask();
+ callOnMainThread([resource = WTFMove(_resource)] { });
+ }
+
</ins><span class="cx"> [super dealloc];
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>