Bug 227863 – [curl] Can't finish loading some websites after r271946 started throttling low priority resources

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

[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

[Fujii Hironori]

Created attachment 433293 [details]
WIP patch

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 433300 [details]
Patch

[Basuke Suzuki]

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]

(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]

> 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 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]

(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 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]

Created attachment 433380 [details]
Patch

[EWS]

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]

<rdar://problem/80499301>