[Fetch API] Request constructor should provide exception messages
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Oct 2016 14:49:47 +0000 (14:49 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Oct 2016 14:49:47 +0000 (14:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162382

Source/WebCore:

Patch by Youenn Fablet <youenn@apple.com> on 2016-10-08
Reviewed by Darin Adler.

No change of behavior, except that exceptions now have error messages.

Added support of exception messages to ExceptionOr.
Making use of ExceptionOr for Request constructor parameter checking.

* Modules/fetch/FetchRequest.cpp:
(WebCore::setReferrerPolicy):
(WebCore::setMode):
(WebCore::setCredentials):
(WebCore::setCache):
(WebCore::setRedirect):
(WebCore::setMethod):
(WebCore::setReferrer):
(WebCore::buildOptions):
(WebCore::FetchRequest::initializeOptions):
(WebCore::FetchRequest::initializeWith):
* Modules/fetch/FetchRequest.h:
* Modules/fetch/FetchRequest.idl:
* bindings/js/JSDOMBinding.cpp:
(WebCore::setDOMException):
* bindings/js/JSDOMBinding.h:
(WebCore::toJS):
(WebCore::toJSNewlyCreated):
* dom/Exception.h:
(WebCore::Exception::code):
(WebCore::Exception::message):
(WebCore::Exception::Exception):
* dom/ExceptionOr.h:
(WebCore::ExceptionOr<ReturnType>::exceptionMessage):

LayoutTests:

Patch by Youenn Fablet <youennf@gmail.com> on 2016-10-08
Reviewed by Darin Adler.

* fetch/fetch-url-serialization-expected.txt: Rebasing test expectation.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@206954 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fetch/fetch-url-serialization-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/Modules/fetch/FetchRequest.cpp
Source/WebCore/Modules/fetch/FetchRequest.h
Source/WebCore/Modules/fetch/FetchRequest.idl
Source/WebCore/bindings/js/JSDOMBinding.cpp
Source/WebCore/bindings/js/JSDOMBinding.h
Source/WebCore/dom/Exception.h
Source/WebCore/dom/ExceptionOr.h

index 3c15883..6133f8f 100644 (file)
@@ -1,3 +1,12 @@
+2016-10-08  Youenn Fablet  <youennf@gmail.com>
+
+        [Fetch API] Request constructor should provide exception messages
+        https://bugs.webkit.org/show_bug.cgi?id=162382
+
+        Reviewed by Darin Adler.
+
+        * fetch/fetch-url-serialization-expected.txt: Rebasing test expectation.
+
 2016-10-07  Chris Dumez  <cdumez@apple.com>
 
         window.navigator.language incorrectly returns all lowercase string
index 60e5584..470eb51 100644 (file)
@@ -97,7 +97,7 @@ FAIL Testing Request url 'file:c:\foo\bar.html' with base 'file:///tmp/mock/path
 FAIL Testing Request url '  File:c|////foo\bar.html' with base 'file:///tmp/mock/path' assert_equals: expected "file:///c:////foo/bar.html" but got "file:///tmp/mock/c|////foo/bar.html"
 FAIL Testing Request url 'C|/foo/bar' with base 'file:///tmp/mock/path' assert_equals: expected "file:///C:/foo/bar" but got "file:///tmp/mock/C|/foo/bar"
 FAIL Testing Request url '/C|\foo\bar' with base 'file:///tmp/mock/path' assert_equals: expected "file:///C:/foo/bar" but got "file:///C|/foo/bar"
-FAIL Testing Request url '//C|/foo/bar' with base 'file:///tmp/mock/path' Type error
+FAIL Testing Request url '//C|/foo/bar' with base 'file:///tmp/mock/path' URL is not valid or contains user credentials.
 PASS Testing Request url '//server/file' with base 'file:///tmp/mock/path' 
 PASS Testing Request url '\\server\file' with base 'file:///tmp/mock/path' 
 PASS Testing Request url '/\server/file' with base 'file:///tmp/mock/path' 
index 01424a9..cc0bb33 100644 (file)
@@ -1,5 +1,42 @@
 2016-10-08  Youenn Fablet  <youenn@apple.com>
 
+        [Fetch API] Request constructor should provide exception messages
+        https://bugs.webkit.org/show_bug.cgi?id=162382
+
+        Reviewed by Darin Adler.
+
+        No change of behavior, except that exceptions now have error messages.
+
+        Added support of exception messages to ExceptionOr.
+        Making use of ExceptionOr for Request constructor parameter checking.
+
+        * Modules/fetch/FetchRequest.cpp:
+        (WebCore::setReferrerPolicy):
+        (WebCore::setMode):
+        (WebCore::setCredentials):
+        (WebCore::setCache):
+        (WebCore::setRedirect):
+        (WebCore::setMethod):
+        (WebCore::setReferrer):
+        (WebCore::buildOptions):
+        (WebCore::FetchRequest::initializeOptions):
+        (WebCore::FetchRequest::initializeWith):
+        * Modules/fetch/FetchRequest.h:
+        * Modules/fetch/FetchRequest.idl:
+        * bindings/js/JSDOMBinding.cpp:
+        (WebCore::setDOMException):
+        * bindings/js/JSDOMBinding.h:
+        (WebCore::toJS):
+        (WebCore::toJSNewlyCreated):
+        * dom/Exception.h:
+        (WebCore::Exception::code):
+        (WebCore::Exception::message):
+        (WebCore::Exception::Exception):
+        * dom/ExceptionOr.h:
+        (WebCore::ExceptionOr<ReturnType>::exceptionMessage):
+
+2016-10-08  Youenn Fablet  <youenn@apple.com>
+
         Refactor binding generated casted-this checks
         https://bugs.webkit.org/show_bug.cgi?id=162677
 
index 80f8b11..254a0ff 100644 (file)
@@ -39,7 +39,7 @@
 
 namespace WebCore {
 
-static bool setReferrerPolicy(FetchOptions& options, const String& referrerPolicy)
+static Optional<Exception> setReferrerPolicy(FetchOptions& options, const String& referrerPolicy)
 {
     if (referrerPolicy.isEmpty())
         options.referrerPolicy = FetchOptions::ReferrerPolicy::EmptyString;
@@ -54,11 +54,11 @@ static bool setReferrerPolicy(FetchOptions& options, const String& referrerPolic
     else if (referrerPolicy == "unsafe-url")
         options.referrerPolicy = FetchOptions::ReferrerPolicy::UnsafeUrl;
     else
-        return false;
-    return true;
+        return Exception { TypeError, ASCIILiteral("Bad referrer policy value.") };
+    return Nullopt;
 }
 
-static bool setMode(FetchOptions& options, const String& mode)
+static Optional<Exception> setMode(FetchOptions& options, const String& mode)
 {
     if (mode == "navigate")
         options.mode = FetchOptions::Mode::Navigate;
@@ -69,11 +69,11 @@ static bool setMode(FetchOptions& options, const String& mode)
     else if (mode == "cors")
         options.mode = FetchOptions::Mode::Cors;
     else
-        return false;
-    return true;
+        return Exception { TypeError, ASCIILiteral("Bad fetch mode value.") };
+    return Nullopt;
 }
 
-static bool setCredentials(FetchOptions& options, const String& credentials)
+static Optional<Exception> setCredentials(FetchOptions& options, const String& credentials)
 {
     if (credentials == "omit")
         options.credentials = FetchOptions::Credentials::Omit;
@@ -82,11 +82,11 @@ static bool setCredentials(FetchOptions& options, const String& credentials)
     else if (credentials == "include")
         options.credentials = FetchOptions::Credentials::Include;
     else
-        return false;
-    return true;
+        return Exception { TypeError, ASCIILiteral("Bad credentials mode value.") };
+    return Nullopt;
 }
 
-static bool setCache(FetchOptions& options, const String& cache)
+static Optional<Exception> setCache(FetchOptions& options, const String& cache)
 {
     if (cache == "default")
         options.cache = FetchOptions::Cache::Default;
@@ -99,11 +99,11 @@ static bool setCache(FetchOptions& options, const String& cache)
     else if (cache == "force-cache")
         options.cache = FetchOptions::Cache::ForceCache;
     else
-        return false;
-    return true;
+        return Exception { TypeError, ASCIILiteral("Bad cache mode value.") };
+    return Nullopt;
 }
 
-static bool setRedirect(FetchOptions& options, const String& redirect)
+static Optional<Exception> setRedirect(FetchOptions& options, const String& redirect)
 {
     if (redirect == "follow")
         options.redirect = FetchOptions::Redirect::Follow;
@@ -112,85 +112,103 @@ static bool setRedirect(FetchOptions& options, const String& redirect)
     else if (redirect == "manual")
         options.redirect = FetchOptions::Redirect::Manual;
     else
-        return false;
-    return true;
+        return Exception { TypeError, ASCIILiteral("Bad redirect mode value.") };
+    return Nullopt;
 }
 
-static bool setMethod(ResourceRequest& request, const String& initMethod)
+static Optional<Exception> setMethod(ResourceRequest& request, const String& initMethod)
 {
     if (!isValidHTTPToken(initMethod))
-        return false;
+        return Exception { TypeError, ASCIILiteral("Method is not a valid HTTP token.") };
 
     String method = initMethod.convertToASCIIUppercase();
     if (method == "CONNECT" || method == "TRACE" || method == "TRACK")
-        return false;
+        return Exception { TypeError, ASCIILiteral("Method is forbidden.") };
 
     request.setHTTPMethod((method == "DELETE" || method == "GET" || method == "HEAD" || method == "OPTIONS" || method == "POST" || method == "PUT") ? method : initMethod);
 
-    return true;
+    return Nullopt;
 }
 
-static bool setReferrer(FetchRequest::InternalRequest& request, ScriptExecutionContext& context, const Dictionary& init)
+static Optional<Exception> setReferrer(FetchRequest::InternalRequest& request, ScriptExecutionContext& context, const Dictionary& init)
 {
     String referrer;
     if (!init.get("referrer", referrer))
-        return true;
+        return Nullopt;
     if (referrer.isEmpty()) {
         request.referrer = ASCIILiteral("no-referrer");
-        return true;
+        return Nullopt;
     }
     // FIXME: Tighten the URL parsing algorithm according https://url.spec.whatwg.org/#concept-url-parser.
     URL referrerURL = context.completeURL(referrer);
     if (!referrerURL.isValid())
-        return false;
+        return Exception { TypeError, ASCIILiteral("Referrer is not a valid URL.") };
 
     if (referrerURL.protocolIs("about") && referrerURL.path() == "client") {
         request.referrer = ASCIILiteral("client");
-        return true;
+        return Nullopt;
     }
 
     if (!(context.securityOrigin() && context.securityOrigin()->canRequest(referrerURL)))
-        return false;
+        return Exception { TypeError, ASCIILiteral("Referrer is not same-origin.") };
 
     request.referrer = referrerURL.string();
-    return true;
+    return Nullopt;
 }
 
-static bool buildOptions(FetchRequest::InternalRequest& request, ScriptExecutionContext& context, const Dictionary& init)
+static Optional<Exception> buildOptions(FetchRequest::InternalRequest& request, ScriptExecutionContext& context, const Dictionary& init)
 {
     JSC::JSValue window;
     if (init.get("window", window)) {
         if (!window.isNull())
-            return false;
+            return Exception { TypeError, ASCIILiteral("Window can only be null.") };
     }
 
-    if (!setReferrer(request, context, init))
-        return false;
+    auto exception = setReferrer(request, context, init);
+    if (exception)
+        return exception;
 
     String value;
-    if (init.get("referrerPolicy", value) && !setReferrerPolicy(request.options, value))
-        return false;
+    if (init.get("referrerPolicy", value)) {
+        exception = setReferrerPolicy(request.options, value);
+        if (exception)
+            return exception;
+    }
 
-    if (init.get("mode", value) && !setMode(request.options, value))
-        return false;
+    if (init.get("mode", value)) {
+        exception = setMode(request.options, value);
+        if (exception)
+            return exception;
+    }
     if (request.options.mode == FetchOptions::Mode::Navigate)
-        return false;
+        return Exception { TypeError, ASCIILiteral("Request constructor does not accept navigate fetch mode.") };
 
-    if (init.get("credentials", value) && !setCredentials(request.options, value))
-        return false;
+    if (init.get("credentials", value)) {
+        exception = setCredentials(request.options, value);
+        if (exception)
+            return exception;
+    }
 
-    if (init.get("cache", value) && !setCache(request.options, value))
-        return false;
+    if (init.get("cache", value)) {
+        exception = setCache(request.options, value);
+        if (exception)
+            return exception;
+    }
 
-    if (init.get("redirect", value) && !setRedirect(request.options, value))
-        return false;
+    if (init.get("redirect", value)) {
+        exception = setRedirect(request.options, value);
+        if (exception)
+            return exception;
+    }
 
     init.get("integrity", request.integrity);
 
-    if (init.get("method", value) && !setMethod(request.request, value))
-        return false;
-
-    return true;
+    if (init.get("method", value)) {
+        exception = setMethod(request.request, value);
+        if (exception)
+            return exception;
+    }
+    return Nullopt;
 }
 
 static bool methodCanHaveBody(const FetchRequest::InternalRequest& internalRequest)
@@ -198,58 +216,49 @@ static bool methodCanHaveBody(const FetchRequest::InternalRequest& internalReque
     return internalRequest.request.httpMethod() != "GET" && internalRequest.request.httpMethod() != "HEAD";
 }
 
-void FetchRequest::initializeOptions(const Dictionary& init, ExceptionCode& ec)
+ExceptionOr<Ref<FetchHeaders>> FetchRequest::initializeOptions(const Dictionary& init)
 {
     ASSERT(scriptExecutionContext());
-    if (!buildOptions(m_internalRequest, *scriptExecutionContext(), init)) {
-        ec = TypeError;
-        return;
-    }
+
+    auto exception = buildOptions(m_internalRequest, *scriptExecutionContext(), init);
+    if (exception)
+        return WTFMove(exception.value());
 
     if (m_internalRequest.options.mode == FetchOptions::Mode::NoCors) {
         const String& method = m_internalRequest.request.httpMethod();
-        if (method != "GET" && method != "POST" && method != "HEAD") {
-            ec = TypeError;
-            return;
-        }
-        if (!m_internalRequest.integrity.isEmpty()) {
-            ec = TypeError;
-            return;
-        }
+        if (method != "GET" && method != "POST" && method != "HEAD")
+            return Exception { TypeError, ASCIILiteral("Method must be GET, POST or HEAD in no-cors mode.") };
+        if (!m_internalRequest.integrity.isEmpty())
+            return Exception { TypeError, ASCIILiteral("There cannot be an integrity in no-cors mode.") };
         m_headers->setGuard(FetchHeaders::Guard::RequestNoCors);
     }
+    return m_headers.copyRef();
 }
 
-FetchHeaders* FetchRequest::initializeWith(const String& url, const Dictionary& init, ExceptionCode& ec)
+ExceptionOr<Ref<FetchHeaders>> FetchRequest::initializeWith(const String& url, const Dictionary& init)
 {
     ASSERT(scriptExecutionContext());
     // FIXME: Tighten the URL parsing algorithm according https://url.spec.whatwg.org/#concept-url-parser.
     URL requestURL = scriptExecutionContext()->completeURL(url);
-    if (!requestURL.isValid() || !requestURL.user().isEmpty() || !requestURL.pass().isEmpty()) {
-        ec = TypeError;
-        return nullptr;
-    }
+    if (!requestURL.isValid() || !requestURL.user().isEmpty() || !requestURL.pass().isEmpty())
+        return Exception { TypeError, ASCIILiteral("URL is not valid or contains user credentials.") };
 
     m_internalRequest.options.mode = Mode::Cors;
     m_internalRequest.options.credentials = Credentials::Omit;
     m_internalRequest.referrer = ASCIILiteral("client");
     m_internalRequest.request.setURL(requestURL);
 
-    initializeOptions(init, ec);
-    return m_headers.ptr();
+    return initializeOptions(init);
 }
 
-FetchHeaders* FetchRequest::initializeWith(FetchRequest& input, const Dictionary& init, ExceptionCode& ec)
+ExceptionOr<Ref<FetchHeaders>> FetchRequest::initializeWith(FetchRequest& input, const Dictionary& init)
 {
-    if (input.isDisturbedOrLocked()) {
-        ec = TypeError;
-        return nullptr;
-    }
+    if (input.isDisturbedOrLocked())
+        return Exception {TypeError, ASCIILiteral("Request input is disturbed or locked.") };
 
     m_internalRequest = input.m_internalRequest;
 
-    initializeOptions(init, ec);
-    return m_headers.ptr();
+    return initializeOptions(init);
 }
 
 void FetchRequest::setBody(JSC::ExecState& execState, JSC::JSValue body, FetchRequest* request, ExceptionCode& ec)
index f922c12..434646f 100644 (file)
 
 #if ENABLE(FETCH_API)
 
+#include "ExceptionOr.h"
 #include "FetchBodyOwner.h"
 #include "FetchOptions.h"
 #include "ResourceRequest.h"
+#include <wtf/Optional.h>
 
 namespace WebCore {
 
@@ -46,8 +48,8 @@ class FetchRequest final : public FetchBodyOwner {
 public:
     static Ref<FetchRequest> create(ScriptExecutionContext& context) { return adoptRef(*new FetchRequest(context, Nullopt, FetchHeaders::create(FetchHeaders::Guard::Request), { })); }
 
-    FetchHeaders* initializeWith(FetchRequest&, const Dictionary&, ExceptionCode&);
-    FetchHeaders* initializeWith(const String&, const Dictionary&, ExceptionCode&);
+    ExceptionOr<Ref<FetchHeaders>> initializeWith(FetchRequest&, const Dictionary&);
+    ExceptionOr<Ref<FetchHeaders>> initializeWith(const String&, const Dictionary&);
     void setBody(JSC::ExecState&, JSC::JSValue, FetchRequest*, ExceptionCode&);
 
     const String& method() const { return m_internalRequest.request.httpMethod(); }
@@ -96,7 +98,7 @@ public:
 private:
     FetchRequest(ScriptExecutionContext&, Optional<FetchBody>&&, Ref<FetchHeaders>&&, InternalRequest&&);
 
-    void initializeOptions(const Dictionary&, ExceptionCode&);
+    ExceptionOr<Ref<FetchHeaders>> initializeOptions(const Dictionary&);
 
     // ActiveDOMObject API.
     const char* activeDOMObjectName() const final;
index 5cfe1d2..cee212e 100644 (file)
@@ -62,8 +62,8 @@ interface FetchRequest {
 
     [NewObject, CallWith=ScriptExecutionContext, MayThrowLegacyException] FetchRequest clone();
 
-    [PrivateIdentifier, MayThrowLegacyException] FetchHeaders initializeWith(FetchRequest input, Dictionary init);
-    [PrivateIdentifier, MayThrowLegacyException] FetchHeaders initializeWith(DOMString input, Dictionary init);
+    [PrivateIdentifier, NewObject] FetchHeaders initializeWith(FetchRequest input, Dictionary init);
+    [PrivateIdentifier, NewObject] FetchHeaders initializeWith(DOMString input, Dictionary init);
     [PrivateIdentifier, MayThrowLegacyException, CallWith=ScriptState] void setBody(any body, FetchRequest? request);
 };
 FetchRequest implements FetchBody;
index 8ffb414..6a8a6e4 100644 (file)
@@ -376,6 +376,17 @@ void setDOMException(ExecState* exec, ExceptionCode ec)
     throwDOMException(exec, scope, ec);
 }
 
+void setDOMException(ExecState* exec, ExceptionCode ec, const String& message)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    if (!ec || scope.exception())
+        return;
+
+    throwException(exec, scope, createDOMException(exec, ec, message));
+}
+
 void setDOMException(ExecState* exec, const ExceptionCodeWithMessage& ec)
 {
     VM& vm = exec->vm();
index 573652d..c3b9025 100644 (file)
@@ -187,6 +187,7 @@ JSC::JSValue createDOMException(JSC::ExecState*, ExceptionCode, const String&);
 
 // Convert a DOM implementation exception code into a JavaScript exception in the execution state.
 WEBCORE_EXPORT void setDOMException(JSC::ExecState*, ExceptionCode);
+void setDOMException(JSC::ExecState*, ExceptionCode, const String&);
 void setDOMException(JSC::ExecState*, const ExceptionCodeWithMessage&);
 
 WEBCORE_EXPORT void setDOMExceptionSlow(JSC::ExecState*, JSC::ThrowScope&, ExceptionCode);
@@ -946,7 +947,7 @@ template<typename T> inline JSC::JSValue toNullableJSNumber(Optional<T> optional
 template<typename T> inline JSC::JSValue toJS(JSC::ExecState* state, JSDOMGlobalObject* globalObject, ExceptionOr<T>&& value)
 {
     if (UNLIKELY(value.hasException())) {
-        setDOMException(state, value.exceptionCode());
+        setDOMException(state, value.exceptionCode(), value.exceptionMessage());
         return JSC::jsUndefined();
     }
     return toJS(state, globalObject, value.takeReturnValue());
@@ -956,7 +957,7 @@ template<typename T> inline JSC::JSValue toJSNewlyCreated(JSC::ExecState* state,
 {
     // FIXME: It's really annoying to have two of these functions. Should find a way to combine toJS and toJSNewlyCreated.
     if (UNLIKELY(value.hasException())) {
-        setDOMException(state, value.exceptionCode());
+        setDOMException(state, value.exceptionCode(), value.exceptionMessage());
         return JSC::jsUndefined();
     }
     return toJSNewlyCreated(state, globalObject, value.takeReturnValue());
index bc39523..882630d 100644 (file)
@@ -26,6 +26,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #pragma once
 
+#include <wtf/text/WTFString.h>
+
 namespace WebCore {
 
 using ExceptionCode = int;
@@ -33,11 +35,14 @@ using ExceptionCode = int;
 class Exception {
 public:
     explicit Exception(ExceptionCode);
+    explicit Exception(ExceptionCode, const String&);
 
-    ExceptionCode code() const;
+    ExceptionCode code() const { return m_code; }
+    const String& message() const { return m_message; }
 
 private:
     ExceptionCode m_code;
+    String m_message;
 };
 
 inline Exception::Exception(ExceptionCode code)
@@ -45,9 +50,10 @@ inline Exception::Exception(ExceptionCode code)
 {
 }
 
-inline ExceptionCode Exception::code() const
+inline Exception::Exception(ExceptionCode code, const String& message)
+    : m_code(code)
+    , m_message(message)
 {
-    return m_code;
 }
 
 }
index 39c03f5..53a44de 100644 (file)
@@ -38,6 +38,7 @@ public:
 
     bool hasException() const;
     ExceptionCode exceptionCode() const;
+    const String& exceptionMessage() const;
     ReturnType&& takeReturnValue();
 
 private:
@@ -64,6 +65,11 @@ template<typename ReturnType> inline ExceptionCode ExceptionOr<ReturnType>::exce
     return std::experimental::get<Exception>(m_value).code();
 }
 
+template<typename ReturnType> inline const String& ExceptionOr<ReturnType>::exceptionMessage() const
+{
+    return std::experimental::get<Exception>(m_value).message();
+}
+
 template<typename ReturnType> inline ReturnType&& ExceptionOr<ReturnType>::takeReturnValue()
 {
     return std::experimental::get<ReturnType>(WTFMove(m_value));