198993 2019-06-19 00:31:09 -0700 [Curl] CurlRequestScheduler doesn't terminate worker thread in a certain situation. 2019-06-19 21:16:42 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 takashi.komori webkit-unassigned basuke commit-queue don.olmstead fujii webkit-bug-importer oldest_to_newest 1545996 0 takashi.komori 2019-06-19 00:31:09 -0700 When a CurlRequest receives didReceiveDataCallback from libcurl, it append a task for main thread by calling callClient. If CurlRequest::invalidateClient is called before the task is processed, the task will not be processed properly. This is because invalidateClient changes m_client to nullptr and the task appended by CurlRequest::callClient checks if m_client is null. When calling CurlRequest::didReceiveData task is not processed properly, a paused connection could remain. In this case CurlRequestScheduler never releases the client related to it. Eventually this causes that CurlRequestScheduler's worker thread keeps working. This CurlRequest::invalidateClient() is called from the destructor of NetworkDataTaskCurl. To avoid this situation, we should cancel the request before invalidate. 1545998 1 372442 takashi.komori 2019-06-19 00:56:31 -0700 Created attachment 372442 Patch 1546015 2 fujii 2019-06-19 03:38:08 -0700 (In reply to Takashi Komori from comment #0) > When calling CurlRequest::didReceiveData task is not processed properly, a > paused connection could remain. Why can it remain? > In this case CurlRequestScheduler never releases the client related to it. Really? It seems that CurlRequestScheduler calls completeTransfer. 1546062 3 372442 commit-queue 2019-06-19 08:11:36 -0700 Comment on attachment 372442 Patch Clearing flags on attachment: 372442 Committed r246588: <https://trac.webkit.org/changeset/246588> 1546063 4 commit-queue 2019-06-19 08:11:38 -0700 All reviewed patches have been landed. Closing bug. 1546064 5 webkit-bug-importer 2019-06-19 08:12:18 -0700 <rdar://problem/51896162> 1546403 6 takashi.komori 2019-06-19 20:54:18 -0700 (In reply to Fujii Hironori from comment #2) > (In reply to Takashi Komori from comment #0) > > When calling CurlRequest::didReceiveData task is not processed properly, a > > paused connection could remain. > > Why can it remain? > > > In this case CurlRequestScheduler never releases the client related to it. > > Really? It seems that CurlRequestScheduler calls completeTransfer. To be precise, the task added by didreceiveData is not processed properly. And when that task is not processed, the connection rmains paused. CurlRequest::didReceiveData (PAUSE) CurlRequest::invokeDidReceiveResponse | | (Add task by using callClient) | NetworkDataTaskCurl::curlDidReceiveResponse NetworkDataTaskCurl::invokeDidReceiveResponse CurlRequest::completeDidReceiveResponse (UNPAUSE) CrrlRequest::didCompleteTransfer In this case, paused connection won't return CURLMSG_DONE message and CurlRequestScheduler::completeTransfer will not be invoked. 1546412 7 fujii 2019-06-19 21:16:42 -0700 I got the idea. Thanks. 372442 2019-06-19 00:56:31 -0700 2019-06-19 08:11:36 -0700 Patch bug-198993-20190619165415.patch text/plain 1569 takashi.komori U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ2NTU4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDg1YmQ0YWYzZTI5OGNhODEw ZjVhMGU2YmI0OTJiMTZjMjAyNWMxZWYuLmEyMmEwNTczNmU3ZTI3YWFjODc5Njc1MmJmYjQzN2Ix YjEwZjRmYWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTktMDYtMTkgIFRha2FzaGkg S29tb3JpICA8VGFrYXNoaS5Lb21vcmlAc29ueS5jb20+CisKKyAgICAgICAgW0N1cmxdIEN1cmxS ZXF1ZXN0U2NoZWR1bGVyIGRvZXNuJ3QgdGVybWluYXRlIHdvcmtlciB0aHJlYWQgaW4gYSBjZXJ0 YWluIHNpdHVhdGlvbi4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE5ODk5MworCisgICAgICAgIENhbmNlbCBDdXJsUmVxdWVzdCBiZWZvcmUgaW52YWxp ZGF0aW9uIHRvIHJlbW92ZSB0YXNrcyBmcm9tIEN1cmxSZXF1ZXN0U2NoZWR1bGVyIGNlcnRhaW5s eSBpbiB+TmV0d29ya0RhdGFUYXNrQ3VybC4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIE5ldHdvcmtQcm9jZXNzL2N1cmwvTmV0d29ya0RhdGFUYXNr Q3VybC5jcHA6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtEYXRhVGFza0N1cmw6On5OZXR3b3Jr RGF0YVRhc2tDdXJsKToKKwogMjAxOS0wNi0xOCAgWW91ZW5uIEZhYmxldCAgPHlvdWVubkBhcHBs ZS5jb20+CiAKICAgICAgICAgU3RvcmFnZU1hbmFnZXI6OnJlbW92ZUFsbG93ZWRTZXNzaW9uU3Rv cmFnZU5hbWVzcGFjZUNvbm5lY3Rpb24gc2hvdWxkIG1ha2Ugc3VyZSBpdHMgc3RvcmFnZU5hbWVz cGFjZUlEIGlzIHZhbGlkCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNz L2N1cmwvTmV0d29ya0RhdGFUYXNrQ3VybC5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9j ZXNzL2N1cmwvTmV0d29ya0RhdGFUYXNrQ3VybC5jcHAKaW5kZXggMzBmNzk0Zjc1ODRhNTZhMTY4 NDQ4OGQ2YWQ0OWVkNDUwN2RkZDRkNS4uYWQ1ZDlmYTc0MDk0YzY2ZDljMjRjMzI4MGQ0MzI1Yjc3 YmJjNDZlZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jdXJsL05l dHdvcmtEYXRhVGFza0N1cmwuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3Mv Y3VybC9OZXR3b3JrRGF0YVRhc2tDdXJsLmNwcApAQCAtNzYsOCArNzYsNyBAQCBOZXR3b3JrRGF0 YVRhc2tDdXJsOjpOZXR3b3JrRGF0YVRhc2tDdXJsKE5ldHdvcmtTZXNzaW9uJiBzZXNzaW9uLCBO ZXR3b3JrRGF0YVRhcwogCiBOZXR3b3JrRGF0YVRhc2tDdXJsOjp+TmV0d29ya0RhdGFUYXNrQ3Vy bCgpCiB7Ci0gICAgaWYgKG1fY3VybFJlcXVlc3QpCi0gICAgICAgIG1fY3VybFJlcXVlc3QtPmlu dmFsaWRhdGVDbGllbnQoKTsKKyAgICBpbnZhbGlkYXRlQW5kQ2FuY2VsKCk7CiB9CiAKIHZvaWQg TmV0d29ya0RhdGFUYXNrQ3VybDo6cmVzdW1lKCkK