Diff
Modified: trunk/LayoutTests/ChangeLog (244683 => 244684)
--- trunk/LayoutTests/ChangeLog 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/LayoutTests/ChangeLog 2019-04-26 09:52:49 UTC (rev 244684)
@@ -1,3 +1,13 @@
+2019-04-26 Takashi Komori <[email protected]>
+
+ [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
+ https://bugs.webkit.org/show_bug.cgi?id=191650
+
+ Reviewed by Fujii Hironori.
+
+ * http/tests/misc/repeat-open-cancel-expected.txt: Added.
+ * http/tests/misc/repeat-open-cancel.html: Added.
+
2019-04-25 Myles C. Maxfield <[email protected]>
[iOS] Implement idempotent mode for text autosizing
Added: trunk/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt (0 => 244684)
--- trunk/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt 2019-04-26 09:52:49 UTC (rev 244684)
@@ -0,0 +1,5 @@
+This is a test for https://bugs.webkit.org/show_bug.cgi?id=191650.
+
+In case that several requests are closed and created pretty fast, a result of old request sometimes goes to a wrong request.
+
+PASS
Added: trunk/LayoutTests/http/tests/misc/repeat-open-cancel.html (0 => 244684)
--- trunk/LayoutTests/http/tests/misc/repeat-open-cancel.html (rev 0)
+++ trunk/LayoutTests/http/tests/misc/repeat-open-cancel.html 2019-04-26 09:52:49 UTC (rev 244684)
@@ -0,0 +1,52 @@
+<html>
+<head>
+<script>
+var repeatCount = 0;
+
+function sendRequest() {
+ var firstReq = new XMLHttpRequest();
+ firstReq.open("GET", "/resources/download-json-with-delay.php?iteration=100&delay=1");
+
+ firstReq._onreadystatechange_ = function() {
+ if (firstReq.readyState == firstReq.HEADERS_RECEIVED) {
+ var secondReq = new XMLHttpRequest();
+ secondReq.open("GET", "/resources/download-json-with-delay.php?iteration=10&delay=0");
+
+ secondReq._onload_ = function() {
+ if (repeatCount++ < 40)
+ sendRequest();
+ else {
+ document.getElementById('result').innerText = "PASS";
+ testRunner.notifyDone();
+ }
+ }
+
+ secondReq._onerror_ = function() {
+ document.getElementById('result').innerText = "FAIL";
+ testRunner.notifyDone();
+ }
+
+ firstReq.abort();
+ secondReq.send();
+ }
+ }
+
+ firstReq.send();
+}
+
+function runTest() {
+ if (window.testRunner) {
+ testRunner.waitUntilDone();
+ testRunner.dumpAsText();
+ }
+
+ sendRequest();
+}
+</script>
+</head>
+<body _onload_="runTest()">
+<p>This is a test for https://bugs.webkit.org/show_bug.cgi?id=191650.</p>
+<p>In case that several requests are closed and created pretty fast, a result of old request sometimes goes to a wrong request.</p>
+<p id="result">RUNNING</p>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (244683 => 244684)
--- trunk/Source/WebCore/ChangeLog 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/Source/WebCore/ChangeLog 2019-04-26 09:52:49 UTC (rev 244684)
@@ -1,3 +1,37 @@
+2019-04-26 Takashi Komori <[email protected]>
+
+ [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
+ https://bugs.webkit.org/show_bug.cgi?id=191650
+
+ Reviewed by Fujii Hironori.
+
+ Test: http/tests/misc/repeat-open-cancel.html
+
+ * platform/network/curl/CurlRequest.cpp:
+ (WebCore::CurlRequest::cancel):
+ (WebCore::CurlRequest::isCancelled):
+ (WebCore::CurlRequest::isCompletedOrCancelled):
+ (WebCore::CurlRequest::didCompleteTransfer):
+ (WebCore::CurlRequest::completeDidReceiveResponse):
+ (WebCore::CurlRequest::pausedStatusChanged):
+ * platform/network/curl/CurlRequest.h:
+ (WebCore::CurlRequest::isCompleted const): Deleted.
+ (WebCore::CurlRequest::isCancelled const): Deleted.
+ (WebCore::CurlRequest::isCompletedOrCancelled const): Deleted.
+ * platform/network/curl/CurlRequestScheduler.cpp:
+ (WebCore::CurlRequestScheduler::cancel):
+ (WebCore::CurlRequestScheduler::callOnWorkerThread):
+ (WebCore::CurlRequestScheduler::startThreadIfNeeded):
+ (WebCore::CurlRequestScheduler::stopThreadIfNoMoreJobRunning):
+ (WebCore::CurlRequestScheduler::stopThread):
+ (WebCore::CurlRequestScheduler::executeTasks):
+ (WebCore::CurlRequestScheduler::workerThread):
+ (WebCore::CurlRequestScheduler::startTransfer):
+ (WebCore::CurlRequestScheduler::completeTransfer):
+ (WebCore::CurlRequestScheduler::cancelTransfer):
+ (WebCore::CurlRequestScheduler::finalizeTransfer):
+ * platform/network/curl/CurlRequestScheduler.h:
+
2019-04-25 Myles C. Maxfield <[email protected]>
[iOS] Add internal setting to force -webkit-text-size-adjust to "auto"
Modified: trunk/Source/WebCore/platform/network/curl/CurlRequest.cpp (244683 => 244684)
--- trunk/Source/WebCore/platform/network/curl/CurlRequest.cpp 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/Source/WebCore/platform/network/curl/CurlRequest.cpp 2019-04-26 09:52:49 UTC (rev 244684)
@@ -126,10 +126,13 @@
{
ASSERT(isMainThread());
- if (isCompletedOrCancelled())
- return;
+ {
+ auto locker = holdLock(m_statusMutex);
+ if (m_cancelled)
+ return;
- m_cancelled = true;
+ m_cancelled = true;
+ }
auto& scheduler = CurlContext::singleton().scheduler();
@@ -143,6 +146,18 @@
invalidateClient();
}
+bool CurlRequest::isCancelled()
+{
+ auto locker = holdLock(m_statusMutex);
+ return m_cancelled;
+}
+
+bool CurlRequest::isCompletedOrCancelled()
+{
+ auto locker = holdLock(m_statusMutex);
+ return m_completed || m_cancelled;
+}
+
void CurlRequest::suspend()
{
ASSERT(isMainThread());
@@ -415,8 +430,8 @@
void CurlRequest::didCompleteTransfer(CURLcode result)
{
- if (m_cancelled) {
- m_curlHandle = nullptr;
+ if (isCancelled()) {
+ didCancelTransfer();
return;
}
@@ -455,6 +470,11 @@
client.curlDidFailWithError(request, error);
});
}
+
+ {
+ auto locker = holdLock(m_statusMutex);
+ m_completed = true;
+ }
}
void CurlRequest::didCancelTransfer()
@@ -567,12 +587,9 @@
ASSERT(m_didNotifyResponse);
ASSERT(!m_didReturnFromNotify || m_multipartHandle);
- if (isCancelled())
+ if (isCompletedOrCancelled())
return;
- if (m_actionAfterInvoke != Action::StartTransfer && isCompleted())
- return;
-
m_didReturnFromNotify = true;
if (m_actionAfterInvoke == Action::ReceiveData) {
@@ -635,7 +652,7 @@
return;
runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
- if (isCompletedOrCancelled())
+ if (isCompletedOrCancelled() || !m_curlHandle)
return;
bool needCancel { false };
Modified: trunk/Source/WebCore/platform/network/curl/CurlRequest.h (244683 => 244684)
--- trunk/Source/WebCore/platform/network/curl/CurlRequest.h 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/Source/WebCore/platform/network/curl/CurlRequest.h 2019-04-26 09:52:49 UTC (rev 244684)
@@ -83,9 +83,8 @@
WEBCORE_EXPORT void resume();
const ResourceRequest& resourceRequest() const { return m_request; }
- bool isCompleted() const { return !m_curlHandle; }
- bool isCancelled() const { return m_cancelled; }
- bool isCompletedOrCancelled() const { return isCompleted() || isCancelled(); }
+ bool isCancelled();
+ bool isCompletedOrCancelled();
Seconds timeoutInterval() const;
const String& user() const { return m_user; }
@@ -166,7 +165,9 @@
CurlRequestClient* m_client { };
+ Lock m_statusMutex;
bool m_cancelled { false };
+ bool m_completed { false };
MessageQueue<Function<void()>>* m_messageQueue { };
ResourceRequest m_request;
Modified: trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp (244683 => 244684)
--- trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp 2019-04-26 09:52:49 UTC (rev 244684)
@@ -58,16 +58,16 @@
{
ASSERT(isMainThread());
- if (!client || !client->handle())
+ if (!client)
return;
- cancelTransfer(client->handle());
+ cancelTransfer(client);
}
void CurlRequestScheduler::callOnWorkerThread(WTF::Function<void()>&& task)
{
{
- LockHolder locker(m_mutex);
+ auto locker = holdLock(m_mutex);
m_taskQueue.append(WTFMove(task));
}
@@ -78,17 +78,26 @@
{
ASSERT(isMainThread());
- LockHolder locker(m_mutex);
- if (!m_runThread) {
- if (m_thread)
- m_thread->waitForCompletion();
+ {
+ auto locker = holdLock(m_mutex);
+ if (m_runThread)
+ return;
+ }
+ if (m_thread)
+ m_thread->waitForCompletion();
+
+ {
+ auto locker = holdLock(m_mutex);
m_runThread = true;
- m_thread = Thread::create("curlThread", [this] {
- workerThread();
- m_runThread = false;
- });
}
+
+ m_thread = Thread::create("curlThread", [this] {
+ workerThread();
+
+ auto locker = holdLock(m_mutex);
+ m_runThread = false;
+ });
}
void CurlRequestScheduler::stopThreadIfNoMoreJobRunning()
@@ -95,19 +104,19 @@
{
ASSERT(!isMainThread());
- if (m_activeJobs.size())
+ auto locker = holdLock(m_mutex);
+ if (m_activeJobs.size() || m_taskQueue.size())
return;
- LockHolder locker(m_mutex);
- if (m_taskQueue.size())
- return;
-
m_runThread = false;
}
void CurlRequestScheduler::stopThread()
{
- m_runThread = false;
+ {
+ auto locker = holdLock(m_mutex);
+ m_runThread = false;
+ }
if (m_thread) {
m_thread->waitForCompletion();
@@ -122,7 +131,7 @@
Vector<WTF::Function<void()>> taskQueue;
{
- LockHolder locker(m_mutex);
+ auto locker = holdLock(m_mutex);
taskQueue = WTFMove(m_taskQueue);
}
@@ -139,7 +148,13 @@
m_curlMultiHandle->setMaxTotalConnections(m_maxTotalConnections);
m_curlMultiHandle->setMaxHostConnections(m_maxHostConnections);
- while (m_runThread) {
+ while (true) {
+ {
+ auto locker = holdLock(m_mutex);
+ if (!m_runThread)
+ break;
+ }
+
executeTasks();
// Retry 'select' if it was interrupted by a process signal.
@@ -177,7 +192,8 @@
break;
ASSERT(msg->msg == CURLMSG_DONE);
- completeTransfer(msg->easy_handle, msg->data.result);
+ if (auto client = m_clientMaps.inlineGet(msg->easy_handle))
+ completeTransfer(client, msg->data.result);
}
stopThreadIfNoMoreJobRunning();
@@ -192,49 +208,59 @@
auto task = [this, client]() {
CURL* handle = client->setupTransfer();
- if (!handle)
+ if (!handle) {
+ completeTransfer(client, CURLE_FAILED_INIT);
return;
+ }
- m_activeJobs.add(handle, client);
m_curlMultiHandle->addHandle(handle);
+
+ ASSERT(!m_clientMaps.contains(handle));
+ m_clientMaps.set(handle, client);
};
- LockHolder locker(m_mutex);
+ auto locker = holdLock(m_mutex);
+ m_activeJobs.add(client);
m_taskQueue.append(WTFMove(task));
}
-void CurlRequestScheduler::completeTransfer(CURL* handle, CURLcode result)
+void CurlRequestScheduler::completeTransfer(CurlRequestSchedulerClient* client, CURLcode result)
{
- finalizeTransfer(handle, [result](CurlRequestSchedulerClient* client) {
+ finalizeTransfer(client, [client, result]() {
client->didCompleteTransfer(result);
});
}
-void CurlRequestScheduler::cancelTransfer(CURL* handle)
+void CurlRequestScheduler::cancelTransfer(CurlRequestSchedulerClient* client)
{
- finalizeTransfer(handle, [](CurlRequestSchedulerClient* client) {
+ finalizeTransfer(client, [client]() {
client->didCancelTransfer();
});
}
-void CurlRequestScheduler::finalizeTransfer(CURL* handle, Function<void(CurlRequestSchedulerClient*)> completionHandler)
+void CurlRequestScheduler::finalizeTransfer(CurlRequestSchedulerClient* client, Function<void()> completionHandler)
{
- auto task = [this, handle, completion = WTFMove(completionHandler)]() {
- if (!m_activeJobs.contains(handle))
- return;
+ auto locker = holdLock(m_mutex);
- CurlRequestSchedulerClient* client = m_activeJobs.inlineGet(handle);
+ if (!m_activeJobs.contains(client))
+ return;
- m_curlMultiHandle->removeHandle(handle);
- m_activeJobs.remove(handle);
- completion(client);
+ m_activeJobs.remove(client);
+ auto task = [this, client, completionHandler = WTFMove(completionHandler)]() {
+ if (client->handle()) {
+ ASSERT(m_clientMaps.contains(client->handle()));
+ m_clientMaps.remove(client->handle());
+ m_curlMultiHandle->removeHandle(client->handle());
+ }
+
+ completionHandler();
+
callOnMainThread([client]() {
client->release();
});
};
- LockHolder locker(m_mutex);
m_taskQueue.append(WTFMove(task));
}
Modified: trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.h (244683 => 244684)
--- trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.h 2019-04-26 06:39:30 UTC (rev 244683)
+++ trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.h 2019-04-26 09:52:49 UTC (rev 244684)
@@ -59,16 +59,17 @@
void workerThread();
void startTransfer(CurlRequestSchedulerClient*);
- void completeTransfer(CURL*, CURLcode);
- void cancelTransfer(CURL*);
- void finalizeTransfer(CURL*, Function<void(CurlRequestSchedulerClient*)>);
+ void completeTransfer(CurlRequestSchedulerClient*, CURLcode);
+ void cancelTransfer(CurlRequestSchedulerClient*);
+ void finalizeTransfer(CurlRequestSchedulerClient*, Function<void()>);
- mutable Lock m_mutex;
+ Lock m_mutex;
RefPtr<Thread> m_thread;
bool m_runThread { false };
Vector<Function<void()>> m_taskQueue;
- HashMap<CURL*, CurlRequestSchedulerClient*> m_activeJobs;
+ HashSet<CurlRequestSchedulerClient*> m_activeJobs;
+ HashMap<CURL*, CurlRequestSchedulerClient*> m_clientMaps;
std::unique_ptr<CurlMultiHandle> m_curlMultiHandle;
