Attachment 345835 Details for Bug 188033 – PATCH

[patch] PATCH

188033.diff (text/plain), 7.27 KB, created by Basuke Suzuki on 2018-07-26 01:11:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-07-26 01:11:54 PDT

Size: 7.27 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 7245b76c53d..cddd26cc0a7 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-07-25  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Crash on synchronous request via ResourceHandle.
>+        https://bugs.webkit.org/show_bug.cgi?id=188033
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/wincairo/TestExpectations:
>+
> 2018-07-26  Basuke Suzuki  <Basuke.Suzuki@sony.com>
> 
>         [Curl] Test gardening
>diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
>index 0e01d291267..f4c30eff8ed 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -860,7 +860,7 @@ http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failu
> http/tests/websocket/tests/hybi/contentextensions/block-cookies.php [ Timeout ]
> 
> # XMLHTTPRequest (sync)
>-http/tests/xmlhttprequest/simple-sync.html [ Crash ]
>+http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> 
> #///////////////////////////////////////////////////////////////////////////////
> # Issue categories below are shared with other platforms (primarily AppleWin)
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e16c2a630fb..1b5a6116275 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-07-25  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Crash on synchronous request via ResourceHandle.
>+        https://bugs.webkit.org/show_bug.cgi?id=188033
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The timing of instanciation of delegate was wrong. Move it inside
>+        getter() and make its return type from pointer to reference to indicate
>+        it's not null.
>+
>+        Test: http/tests/xmlhttprequest/simple-sync.html
>+
>+        * platform/network/ResourceHandle.h:
>+        * platform/network/curl/ResourceHandleCurl.cpp:
>+        (WebCore::ResourceHandle::start):
>+        (WebCore::ResourceHandle::createCurlRequest):
>+        (WebCore::ResourceHandle::delegate):
>+        (WebCore::ResourceHandle::receivedRequestToContinueWithoutCredential):
>+        (WebCore::ResourceHandle::shouldRedirectAsGET):
>+        (WebCore::ResourceHandle::willSendRequest):
>+        (WebCore::ResourceHandle::continueAfterWillSendRequest):
>+
> 2018-07-25  Chris Dumez  <cdumez@apple.com>
> 
>         Allow ActiveDOMObject's canSuspend / suspend / resume overrides to destroy ActiveDOMObjects
>diff --git a/Source/WebCore/platform/network/ResourceHandle.h b/Source/WebCore/platform/network/ResourceHandle.h
>index fce796cadad..748854edb64 100644
>--- a/Source/WebCore/platform/network/ResourceHandle.h
>+++ b/Source/WebCore/platform/network/ResourceHandle.h
>@@ -173,7 +173,7 @@ public:
> 
> #if USE(CURL)
>     bool cancelledOrClientless();
>-    CurlResourceHandleDelegate* delegate();
>+    CurlResourceHandleDelegate& delegate();
> 
>     void continueAfterDidReceiveResponse();
>     void willSendRequest();
>diff --git a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>index 6384d628909..2bb5dd6b769 100644
>--- a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>@@ -66,9 +66,6 @@ bool ResourceHandle::start()
> 
>     CurlContext::singleton();
> 
>-    if (!d->m_delegate.get())
>-        d->m_delegate = std::make_unique<CurlResourceHandleDelegate>(*this);
>-
>     // The frame could be null if the ResourceHandle is not associated to any
>     // Frame, e.g. if we are downloading a file.
>     // If the frame is not null but the page is null this must be an attempted
>@@ -152,15 +149,17 @@ Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, Req
>     }
> 
>     CurlRequest::ShouldSuspend shouldSuspend = d->m_defersLoading ? CurlRequest::ShouldSuspend::Yes : CurlRequest::ShouldSuspend::No;
>-    auto curlRequest = CurlRequest::create(request, *delegate(), shouldSuspend, CurlRequest::EnableMultipart::Yes);
>+    auto curlRequest = CurlRequest::create(request, delegate(), shouldSuspend, CurlRequest::EnableMultipart::Yes);
>     
>     return curlRequest;
> }
> 
>-CurlResourceHandleDelegate* ResourceHandle::delegate()
>+CurlResourceHandleDelegate& ResourceHandle::delegate()
> {
>-    ASSERT(d->m_delegate);
>-    return d->m_delegate.get();
>+    if (!d->m_delegate)
>+        d->m_delegate = std::make_unique<CurlResourceHandleDelegate>(*this);
>+
>+    return *d->m_delegate.get();
> }
> 
> #if OS(WINDOWS)
>@@ -303,7 +302,7 @@ void ResourceHandle::receivedRequestToContinueWithoutCredential(const Authentica
> 
>     clearAuthentication();
> 
>-    didReceiveResponse(ResourceResponse(delegate()->response()), [this, protectedThis = makeRef(*this)] {
>+    didReceiveResponse(ResourceResponse(delegate().response()), [this, protectedThis = makeRef(*this)] {
>         continueAfterDidReceiveResponse();
>     });
> }
>@@ -435,10 +434,12 @@ bool ResourceHandle::shouldRedirectAsGET(const ResourceRequest& request, bool cr
>     if (!request.url().protocolIsInHTTPFamily())
>         return true;
> 
>-    if (delegate()->response().isSeeOther())
>+    const auto& response = delegate().response();
>+
>+    if (response.isSeeOther())
>         return true;
> 
>-    if ((delegate()->response().isMovedPermanently() || delegate()->response().isFound()) && (request.httpMethod() == "POST"))
>+    if ((response.isMovedPermanently() || response.isFound()) && (request.httpMethod() == "POST"))
>         return true;
> 
>     if (crossOrigin && (request.httpMethod() == "DELETE"))
>@@ -453,13 +454,15 @@ void ResourceHandle::willSendRequest()
> 
>     static const int maxRedirects = 20;
> 
>+    const auto& response = delegate().response();
>+
>     if (d->m_redirectCount++ > maxRedirects) {
>-        client()->didFail(this, ResourceError::httpError(CURLE_TOO_MANY_REDIRECTS, delegate()->response().url()));
>+        client()->didFail(this, ResourceError::httpError(CURLE_TOO_MANY_REDIRECTS, response.url()));
>         return;
>     }
> 
>-    String location = delegate()->response().httpHeaderField(HTTPHeaderName::Location);
>-    URL newURL = URL(delegate()->response().url(), location);
>+    String location = response.httpHeaderField(HTTPHeaderName::Location);
>+    URL newURL = URL(response.url(), location);
>     bool crossOrigin = !protocolHostAndPortAreEqual(d->m_firstRequest.url(), newURL);
> 
>     ResourceRequest newRequest = d->m_firstRequest;
>@@ -486,7 +489,7 @@ void ResourceHandle::willSendRequest()
>         newRequest.clearHTTPOrigin();
>     }
> 
>-    ResourceResponse responseCopy = delegate()->response();
>+    auto responseCopy = delegate().response();
>     client()->willSendRequestAsync(this, WTFMove(newRequest), WTFMove(responseCopy), [this, protectedThis = makeRef(*this)] (ResourceRequest&& request) {
>         continueAfterWillSendRequest(WTFMove(request));
>     });
>@@ -505,7 +508,7 @@ void ResourceHandle::continueAfterWillSendRequest(ResourceRequest&& request)
> 
>     d->m_curlRequest = createCurlRequest(request);
> 
>-    if (protocolHostAndPortAreEqual(request.url(), delegate()->response().url())) {
>+    if (protocolHostAndPortAreEqual(request.url(), delegate().response().url())) {
>         if (auto credential = getCredential(request, true))
>             d->m_curlRequest->setUserPass(credential->first, credential->second);
>     }

Flags:

Hironori.Fujii: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 188033: 345820 | 345826 | 345827 | 345830 | 345835 | 345844 | 345846 | 345858 | 345918 | 345925