[curl] Can't finish loading some websites after r271946 started throttle low priority resources 1. Remove the cache directory manually C:\Users\<username>\AppData\Local\Apple Computer\WebKit 2. Start WinCairo WK2 MiniBrowser 3. Go to https://dictionary.goo.ne.jp/ Actual: Doesn't finish loading, and WebKitNetworkProcess.exe becomes busy Expected: Finish loading
Created attachment 433293 [details] WIP patch
Even though there are active jobs, curl_multi_fdset constantly returned -1 because CURLMOPT_MAX_HOST_CONNECTIONS was set to 6 (CurlDefaultMaxHostConnections) and 6 handles were paused. 17 active jobs 6 paused jobs: https://dictionary.goo.ne.jp/img/nandoku_kanji/promotion/300x90.jpg https://dictionary.goo.ne.jp/img/knowledge/exam_english_word.jpg https://dictionary.goo.ne.jp/img/knowledge/english_column.jpg https://dictionary.goo.ne.jp/img/header/goo.png https://dictionary.goo.ne.jp/img/knowledge/kanji_yugi.jpg https://dictionary.goo.ne.jp/img/dictionary.png 11 non paused jobs: https://dictionary.goo.ne.jp/css/newAttention.css?1586565636 https://dictionary.goo.ne.jp/hyge_modified/js/page.js?1583994318 https://dictionary.goo.ne.jp/img/knowledge/study_incase.jpg https://dictionary.goo.ne.jp/js/suggest.js?1583994318 https://dictionary.goo.ne.jp/img/header/dictionary.png https://dictionary.goo.ne.jp/js/script.js?1583994318 https://dictionary.goo.ne.jp/js/headerFix.js?1583994318 https://dictionary.goo.ne.jp/js/personal.js?1613702118 https://dictionary.goo.ne.jp/hyge_modified/js/sp_header_app_balloon.js?1583994318 https://dictionary.goo.ne.jp/img/knowledge/quote.jpg https://dictionary.goo.ne.jp/css/addLogin.css?1596157312
Created attachment 433300 [details] Patch
Comment on attachment 433300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433300&action=review This is informal review: Looks good to me. One question on cancel(). Isn't it safe to add status check before cancelling the request which did not start yet? It seems harmless, but it is the question of the reader of this code that start() and cancel() are related method and it should be taken care of the status of the flag. Other thing we should take care is there's so many boolean flags in the code and it makes reviewing so hard. For instance, m_shouldSuspend and m_didStart is tied together very closely. It can be merged into tri-state enum like { StartSuspended, WaitingForStart, DidStart }. That may be out of scope of this bug. I'll open a new bug to make the logic clean. > Source/WebCore/platform/network/curl/CurlRequest.cpp:-646 > - m_shouldSuspend = m_isPausedOfRequest = paused; This is good. m_shouldSuspend is used before setup, there's no need to update this. And it makes the purpose of m_shouldSuspend clear.
(In reply to Basuke Suzuki from comment #4) > One question on cancel(). Isn't it safe to add status check before > cancelling the request which did not start yet? It seems harmless, but it is > the question of the reader of this code that start() and cancel() are > related method and it should be taken care of the status of the flag. Good point. Will try to fix. > Other thing we should take care is there's so many boolean flags in the code > and it makes reviewing so hard. For instance, m_shouldSuspend and m_didStart > is tied together very closely. It can be merged into tri-state enum like { > StartSuspended, WaitingForStart, DidStart }. That may be out of scope of > this bug. I'll open a new bug to make the logic clean. Umm, I'm not confident it can properly handle the following four cases. case #1: CurlRequest(ShouldSuspend::No) CurlRequest::start() CurlRequest::suspend() case #2: CurlRequest(ShouldSuspend::No) CurlRequest::suspend() CurlRequest::start() CurlRequest::resume() case #3: CurlRequest(ShouldSuspend::Yes) CurlRequest::start() CurlRequest::resume() case #4: CurlRequest(ShouldSuspend::Yes) CurlRequest::resume() CurlRequest::start()
> Umm, I'm not confident it can properly handle the following four cases. Once it get m_didStart == true, m_shouldSuspend has no effect entirely. So suspend/resume is rewritten like this: void CurlRequest::suspend() { ASSERT(isMainThread()); if (m_didStart) setRequestPaused(true); else m_shouldSuspend = true; } void CurlRequest::resume() { ASSERT(isMainThread()); if (m_didStart) setRequestPaused(false); else { m_shouldSuspend = false; start(); } } Then it is more clear that state to manage is: 1. m_didStart == false && m_shouldSuspend = true 2. m_didStart == false && m_shouldSuspend = false 3. m_didStart == true
Comment on attachment 433300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433300&action=review > Source/WebCore/platform/network/curl/CurlRequest.cpp:113 > + if (m_shouldSuspend) It is safe to check m_didStart to escape early. Obviously it is a logic error to call start() twice, but it's nice to have to make the intent clear. > if (m_shouldSuspend || m_didStart)
(In reply to Basuke Suzuki from comment #6) > > Umm, I'm not confident it can properly handle the following four cases. > > Once it get m_didStart == true, m_shouldSuspend has no effect entirely. So > suspend/resume is rewritten like this: > > void CurlRequest::suspend() > { > ASSERT(isMainThread()); > > if (m_didStart) > setRequestPaused(true); > else > m_shouldSuspend = true; > } > > void CurlRequest::resume() > { > ASSERT(isMainThread()); > > if (m_didStart) > setRequestPaused(false); > else { > m_shouldSuspend = false; > start(); > } > } > > Then it is more clear that state to manage is: > > 1. m_didStart == false && m_shouldSuspend = true > 2. m_didStart == false && m_shouldSuspend = false > 3. m_didStart == true I got it. I will revise the patch with your tri-state idea.
Comment on attachment 433300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433300&action=review >> Source/WebCore/platform/network/curl/CurlRequest.cpp:113 >> + if (m_shouldSuspend) > > It is safe to check m_didStart to escape early. Obviously it is a logic error to call start() twice, but it's nice to have to make the intent clear. Will fix.
Created attachment 433380 [details] Patch
Committed r279864 (239617@main): <https://commits.webkit.org/239617@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433380 [details].
<rdar://problem/80499301>