RESOLVED FIXED 227863
[curl] Can't finish loading some websites after r271946 started throttling low priority resources 
https://bugs.webkit.org/show_bug.cgi?id=227863
Summary [curl] Can't finish loading some websites after r271946 started throttling lo...
Fujii Hironori
Reported 2021-07-11 16:37:20 PDT
[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
Attachments
WIP patch (2.09 KB, patch)
2021-07-11 16:39 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (3.64 KB, patch)
2021-07-12 00:06 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (5.30 KB, patch)
2021-07-12 17:41 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Fujii Hironori
Comment 1 2021-07-11 16:39:14 PDT
Created attachment 433293 [details] WIP patch
Fujii Hironori
Comment 3 2021-07-12 00:06:18 PDT
Created attachment 433300 [details] Patch
Basuke Suzuki
Comment 4 2021-07-12 12:55:00 PDT
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.
Fujii Hironori
Comment 5 2021-07-12 13:35:19 PDT
(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()
Basuke Suzuki
Comment 6 2021-07-12 15:01:16 PDT
> 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
Basuke Suzuki
Comment 7 2021-07-12 15:05:35 PDT
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)
Fujii Hironori
Comment 8 2021-07-12 16:48:38 PDT
(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.
Fujii Hironori
Comment 9 2021-07-12 16:49:53 PDT
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.
Fujii Hironori
Comment 10 2021-07-12 17:41:29 PDT
Created attachment 433380 [details] Patch
EWS
Comment 11 2021-07-12 21:39:35 PDT
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].
Radar WebKit Bug Importer
Comment 12 2021-07-12 21:40:18 PDT
<rdar://problem/80499301>
Note You need to log in before you can comment on or make changes to this bug.