Bug 183089 – [Curl] Bug fix on suspend/resume behavior.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

1. Asser error occured when suspend called in CurlRequest::setupTransfer().
   Because CurlRequest::setupTransfer() is called on worker thread.

2. CurlRequest that construct with enabled defferLoading flag doesn't resume
   when resume called before CurlRequest::setupTransfer().
   Because m_shouldSuspend flag doesn't change after construct.

[Basuke Suzuki]

Created attachment 334541 [details]
patch

Bug fix

[Don Olmstead]

Comment on attachment 334541 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334541&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:141
> +    m_shouldSuspend = false;
>      setRequestPaused(false);

Looks fine just curious if the m_shouldSuspend should live in setRequestPaused

[Alex Christensen]

Comment on attachment 334541 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334541&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:-132
> -    ASSERT(isMainThread());

Could this be changed to ASSERT(!isMainThread()); ?
It seems like suspend and resume should be called on the main thread.

[Basuke Suzuki]

Comment on attachment 334541 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334541&action=review

Thank you for reviewing.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:-132
>> -    ASSERT(isMainThread());
> 
> Could this be changed to ASSERT(!isMainThread()); ?
> It seems like suspend and resume should be called on the main thread.

suspend() will be called from background network thread when the request is anync. It will be called when the request is sync.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:141
>>      setRequestPaused(false);
> 
> Looks fine just curious if the m_shouldSuspend should live in setRequestPaused

There's a situation setRequestPaused is called without changing the flag.

[Alex Christensen]

Comment on attachment 334541 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334541&action=review

>>> Source/WebCore/platform/network/curl/CurlRequest.cpp:-132
>>> -    ASSERT(isMainThread());
>> 
>> Could this be changed to ASSERT(!isMainThread()); ?
>> It seems like suspend and resume should be called on the main thread.
> 
> suspend() will be called from background network thread when the request is anync. It will be called when the request is sync.

Suspend and resume ought to mirror each other.  I think something is wrong if they don't.

[Basuke Suzuki]

Created attachment 339887 [details]
FIX

Keep existing suspend/resume pair as is. Change to call internal state change method. Also fix updating the flag for initial suspension.

[youenn fablet]

Comment on attachment 339887 [details]
FIX

View in context: https://bugs.webkit.org/attachment.cgi?id=339887&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:226
> +        setRequestPaused(true);

I guess this change is due to the call being sometimes not done in the main thread.
Is it right?
If so, I wonder whether this code is thread-safe?
Can there be case where setRequestPaused(true) and setRequestPaused(false) are called in the same time in a different thread?
Maybe setupTransfer should always be called in the main thread?


I also wonder whether we can have some tests related to those changes.

[Basuke Suzuki]

Created attachment 340529 [details]
PATCH

Added mutex to protect the status change conflict.

[Basuke Suzuki]

Created attachment 340530 [details]
FIX spacing

[youenn fablet]

Comment on attachment 340530 [details]
FIX spacing

View in context: https://bugs.webkit.org/attachment.cgi?id=340530&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:116
> +            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {

By doing so, there is a chance that CurlRequest will be destroyed in the worker thread.
Is it safe to do so?

> Source/WebCore/platform/network/curl/CurlRequest.cpp:154
> +void CurlRequest::runOnMainThread(WTF::Function<void()>&& task)

It does not need to be a method, a static function would be good enough.

[Basuke Suzuki]

Created attachment 340621 [details]
Add descriptive comment

[Basuke Suzuki]

Oh, I didn't notice about your review. I'm fixing that.

[Basuke Suzuki]

(In reply to youenn fablet from comment #10)
> Comment on attachment 340530 [details]
> FIX spacing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340530&action=review
> 
> > Source/WebCore/platform/network/curl/CurlRequest.cpp:116
> > +            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
> 
> By doing so, there is a chance that CurlRequest will be destroyed in the
> worker thread.
> Is it safe to do so?

Do you mean from the resource management perspective? If so, that's what we want them to do. Things are prepared in setupTransfer() method except m_request and m_formDataStream which is isolated form others. The method runs in worker thread. It make sense they destroyed in background.

[Basuke Suzuki]

Created attachment 340625 [details]
PATCH

Make the method to static function and fix a typo.

[youenn fablet]

Comment on attachment 340625 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=340625&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:41
> +static void runOnMainThread(WTF::Function<void()>&& task);

No need for WTF::

> Source/WebCore/platform/network/curl/CurlRequest.cpp:148
>      setRequestPaused(false);

Maybe these if(isCompletedOrCancelled()) should be done inside setRequestPaused

> Source/WebCore/platform/network/curl/CurlRequest.cpp:157
>              task(*this, *client);

Would the following work:
task(*this, makeRef(*client))

> Source/WebCore/platform/network/curl/CurlRequest.cpp:376
> +            m_handlePaused = true;

I understand that returning CURL_WRITEFUNC_PAUSE will make curl handle be paused, right?
If so, that is why m_handlePaused is set to true, right?
setCallbackPaused(true) is the only needed to update m_isPausedCallback?

That seems okayish but a bit difficult to understand.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:603
>              didCompleteTransfer(m_finishedResultCode);

If you have a lot this pattern:
if (!m_isSyncRequest)
   runOnWorkerThread(WTFMove(a))
else
  a();
Maybe it could be made as a method.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:610
> +        LockHolder lock(m_pauseStateMutex);

It seems that isPaused is only used in worker thread.
If m_shouldSuspend and m_isPausedOfRequest are only useful in worker thread, maybe it is just better to use runOnWorkerThreadIfRequired inside setRequestPaused/setCallbackPaused to do the actual state updates.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:644
> +    */

We usually use // instead of /* */

> Source/WebCore/platform/network/curl/CurlRequest.cpp:672
> +            invokeCancel();

Maybe you can just write:
runOnMainThread([protectedThis = WTFMove(protectedThis)]() {
     protectedThis->cancel();
});

You would need to make the embedding lambda mutable

> Source/WebCore/platform/network/curl/CurlRequest.h:109
> +    void runOnWorkerThreadIfRequired(WTF::Function<void()>&&);

No longer need for WTF::

> Source/WebCore/platform/network/curl/CurlRequest.h:185
> +    bool m_handlePaused { false };

I would rename it to m_isHandlePaused.
Since you want to make sure it is only accessed in worker thread, I would tend to add r/w accessors with debug assertions in it.
That could make the model clearer.

[EWS Watchlist]

Comment on attachment 340625 [details]
PATCH

Attachment 340625 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7717873

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 340674 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Comment on attachment 340625 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=340625&action=review

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:148
>>      setRequestPaused(false);
> 
> Maybe these if(isCompletedOrCancelled()) should be done inside setRequestPaused

start/suspend/resume/cancel are the gateway from main thread and I think it should be guarded close to those point.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:157
>>              task(*this, *client);
> 
> Would the following work:
> task(*this, makeRef(*client))

Oh nice.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:376
>> +            m_handlePaused = true;
> 
> I understand that returning CURL_WRITEFUNC_PAUSE will make curl handle be paused, right?
> If so, that is why m_handlePaused is set to true, right?
> setCallbackPaused(true) is the only needed to update m_isPausedCallback?
> 
> That seems okayish but a bit difficult to understand.

I know. We internally discussed about this very hard and concluded we should send a patch to libcurl repository so that we don't need to manage the state aside with handle. Until then, we keep this ugly . Also good to put a comment here to describe our intent.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:603
>>              didCompleteTransfer(m_finishedResultCode);
> 
> If you have a lot this pattern:
> if (!m_isSyncRequest)
>    runOnWorkerThread(WTFMove(a))
> else
>   a();
> Maybe it could be made as a method.

There're only two places this pattern appears. Other similar places are slightly different. I am planning to separate the class into CurlSyncRequest and CurlAsyncRequest so that I want to keep them as is.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:610
>> +        LockHolder lock(m_pauseStateMutex);
> 
> It seems that isPaused is only used in worker thread.
> If m_shouldSuspend and m_isPausedOfRequest are only useful in worker thread, maybe it is just better to use runOnWorkerThreadIfRequired inside setRequestPaused/setCallbackPaused to do the actual state updates.

Let me think. Then we can remove mutex. The communication is paused by two reason. The upper layer's request by suspend/resume (and deferredLoading), and paused to wait client callback result. It is possible that the request to pause may cause no change at all (the return case in the setRequestPaused and setCallbackPaused). We can avoid sending function invocation to worker thread with current mutex implementation. I don't know how often request suspend/resume happens, but the answer may depend on that answer, how often such request happens.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:672
>> +            invokeCancel();
> 
> Maybe you can just write:
> runOnMainThread([protectedThis = WTFMove(protectedThis)]() {
>      protectedThis->cancel();
> });
> 
> You would need to make the embedding lambda mutable

MSVC hate that code. They cannot compile that. That's why we need this workaround. We are waiting their fix for a while, but still it seems we need more time.

>> Source/WebCore/platform/network/curl/CurlRequest.h:185
>> +    bool m_handlePaused { false };
> 
> I would rename it to m_isHandlePaused.
> Since you want to make sure it is only accessed in worker thread, I would tend to add r/w accessors with debug assertions in it.
> That could make the model clearer.

Make sense.

[Basuke Suzuki]

Created attachment 340683 [details]
Fixed

[EWS Watchlist]

Comment on attachment 340683 [details]
Fixed

Attachment 340683 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7721544

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 340691 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

> > Maybe you can just write:
> > runOnMainThread([protectedThis = WTFMove(protectedThis)]() {
> >      protectedThis->cancel();
> > });
> > 
> > You would need to make the embedding lambda mutable
> 
> MSVC hate that code. They cannot compile that. That's why we need this
> workaround. We are waiting their fix for a while, but still it seems we need
> more time.

I thought the issue was about 'this' in lambda.
If so, given that the above code is not making use of 'this', MSVC must be just fine.

[youenn fablet]

Comment on attachment 340683 [details]
Fixed

r=me 

I am not sure using mutex is the right way of doing things.
Please consider whether posting task to the background thread might not be a clearer and stronger approach.

View in context: https://bugs.webkit.org/attachment.cgi?id=340683&action=review

> Source/WebCore/platform/network/curl/CurlRequest.h:188
> +    bool isHandlePaused() const;

I would put these getters above, not in the middle of member definitions.

[Basuke Suzuki]

(In reply to youenn fablet from comment #22)
> > > Maybe you can just write:
> > > runOnMainThread([protectedThis = WTFMove(protectedThis)]() {
> > >      protectedThis->cancel();
> > > });
> > > 
> > > You would need to make the embedding lambda mutable
> > 
> > MSVC hate that code. They cannot compile that. That's why we need this
> > workaround. We are waiting their fix for a while, but still it seems we need
> > more time.
> 
> I thought the issue was about 'this' in lambda.
> If so, given that the above code is not making use of 'this', MSVC must be
> just fine.

Unfortunately no. Even this simpler method cannot be compiled:

void CurlRequest::hello()
{
    auto externalTask = [protectedThis = makeRef(*this)]() {
        auto inernalTask = [protectedThat = makeRef(*protectedThis)]() {
            protectedThat->cancel();
        };
    };
}

d:\src\a.cpp(652): error C2100:
    illegal indirection [D:\WebCore.vcxproj]
d:\src\a.cpp(652): error C2119:
    'protectedThat': the type for 'auto' can not be deduced from an empty initializer [d:\WebCore.vcxproj]
d:\src\a.cpp(653): error C2227:
    left of '->cancel' must point to class/s truct/union/generic type [d:\\WebCore.vcxproj]

[Basuke Suzuki]

Created attachment 340713 [details]
PATCH

youenn, thanks for r+!

[WebKit Commit Bot]

Comment on attachment 340713 [details]
PATCH

Clearing flags on attachment: 340713

Committed r231968: <https://trac.webkit.org/changeset/231968>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40371954>