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.
Created attachment 334541 [details] patch Bug fix
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
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.
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.
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.
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.
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.
Created attachment 340529 [details] PATCH Added mutex to protect the status change conflict.
Created attachment 340530 [details] FIX spacing
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.
Created attachment 340621 [details] Add descriptive comment
Oh, I didn't notice about your review. I'm fixing that.
(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.
Created attachment 340625 [details] PATCH Make the method to static function and fix a typo.
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.
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
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
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.
Created attachment 340683 [details] Fixed
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
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
> > 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.
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.
(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]
Created attachment 340713 [details] PATCH youenn, thanks for r+!
Comment on attachment 340713 [details] PATCH Clearing flags on attachment: 340713 Committed r231968: <https://trac.webkit.org/changeset/231968>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40371954>