Bug 227863

Summary: [curl] Can't finish loading some websites after r271946 started throttling low priority resources
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: PlatformAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: Basuke.Suzuki, don.olmstead, ews-watchlist, galpeter, takashi.komori, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP patch
none
Patch
none
Patch none

Description Fujii Hironori 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
Comment 1 Fujii Hironori 2021-07-11 16:39:14 PDT 
Created attachment 433293 [details]
WIP patch
Comment 3 Fujii Hironori 2021-07-12 00:06:18 PDT 
Created attachment 433300 [details]
Patch
Comment 4 Basuke Suzuki 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.
Comment 5 Fujii Hironori 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()
Comment 6 Basuke Suzuki 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
Comment 7 Basuke Suzuki 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)
Comment 8 Fujii Hironori 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.
Comment 9 Fujii Hironori 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.
Comment 10 Fujii Hironori 2021-07-12 17:41:29 PDT 
Created attachment 433380 [details]
Patch
Comment 11 EWS 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].
Comment 12 Radar WebKit Bug Importer 2021-07-12 21:40:18 PDT 
<rdar://problem/80499301>