Attachment 345820 Details for Bug 188033 – FIX

[patch] FIX

188033.diff (text/plain), 9.32 KB, created by Basuke Suzuki on 2018-07-25 21:41:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-07-25 21:41:59 PDT

Size: 9.32 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 17621f001c1..4a4989e17a5 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+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!).
>+
>+        Added expected file and make the expectations 'Pass'.
>+
>+        * http/tests/xmlhttprequest/resources/print-referer.php: Added.
>+        * http/tests/xmlhttprequest/simple-sync-expected.txt: Added.
>+        * http/tests/xmlhttprequest/simple-sync.html: Added.
>+        * platform/wincairo/TestExpectations:
>+
> 2018-07-25  David Fenton  <david_fenton@apple.com>
> 
>         Unreviewed, rolling out r234187.
>diff --git a/LayoutTests/http/tests/xmlhttprequest/resources/print-referer.php b/LayoutTests/http/tests/xmlhttprequest/resources/print-referer.php
>new file mode 100644
>index 00000000000..bd198b8c08d
>--- /dev/null
>+++ b/LayoutTests/http/tests/xmlhttprequest/resources/print-referer.php
>@@ -0,0 +1,19 @@
>+<?php
>+
>+header("Content-type: text/plain");
>+echo $_SERVER['HTTP_REFERER'] ?? "NO REFERER";
>+
>+/*
>+
>+#!/usr/bin/perl -w
>+
>+use CGI qw(:standard);
>+my $cgi = new CGI;
>+
>+print "Content-type: text/plain\n\n";
>+if ($cgi->referer) {
>+  print $cgi->referer;
>+} else {
>+  print "NO REFERER";
>+}
>+*/
>diff --git a/LayoutTests/http/tests/xmlhttprequest/simple-sync-expected.txt b/LayoutTests/http/tests/xmlhttprequest/simple-sync-expected.txt
>new file mode 100644
>index 00000000000..d8361cc8f94
>--- /dev/null
>+++ b/LayoutTests/http/tests/xmlhttprequest/simple-sync-expected.txt
>@@ -0,0 +1,3 @@
>+Test for bug 188033: Syncronous XMLHTTPRequest crashes on WinCairo.
>+
>+result: OK
>diff --git a/LayoutTests/http/tests/xmlhttprequest/simple-sync.html b/LayoutTests/http/tests/xmlhttprequest/simple-sync.html
>new file mode 100644
>index 00000000000..d51df7c8ab7
>--- /dev/null
>+++ b/LayoutTests/http/tests/xmlhttprequest/simple-sync.html
>@@ -0,0 +1,19 @@
>+<body>
>+    <p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=188033">bug 188033</a>:
>+    Syncronous XMLHTTPRequest crashes on WinCairo.</p>
>+
>+    <div id=result>result: </div>
>+
>+    <script>
>+    if (window.testRunner)
>+      testRunner.dumpAsText();
>+
>+    req = new XMLHttpRequest;
>+    req.open("GET", "resources/print-referer.php", false);
>+    req.send(null);
>+    if (req.responseText == document.URL)
>+      document.getElementById("result").firstChild.data += "OK";
>+    else
>+      document.getElementById("result").firstChild.data += req.responseText;
>+    </script>
>+</body>
>diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
>index fa209ab5f50..f4c30eff8ed 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -859,6 +859,9 @@ 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 [ Pass ]
>+
> #///////////////////////////////////////////////////////////////////////////////
> # Issue categories below are shared with other platforms (primarily AppleWin)
> #///////////////////////////////////////////////////////////////////////////////
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 1e643d505f9..10066f01ea1 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  David Fenton  <david_fenton@apple.com>
> 
>         Unreviewed, rolling out r234187.
>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:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

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