From 1fe9e0929ecaafea200efdcd297081171ae04681 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Wed, 14 Jan 2015 13:43:27 -0800 Subject: [PATCH] Bug 1121682 - fetch() should reject with TypeError --HG-- extra : rebase_source : 9e8d5e193695b856280c769dccc128c4ce4797d3 --- dom/bindings/Errors.msg | 1 + dom/bindings/Exceptions.cpp | 12 ++ dom/bindings/Exceptions.h | 13 ++ dom/bindings/ToJSValue.cpp | 1 + dom/fetch/Fetch.cpp | 28 +++- .../fetch/worker_test_fetch_basic.js | 4 +- .../fetch/worker_test_fetch_basic_http.js | 5 +- .../mochitest/fetch/worker_test_fetch_cors.js | 154 +++++++----------- 8 files changed, 116 insertions(+), 102 deletions(-) diff --git a/dom/bindings/Errors.msg b/dom/bindings/Errors.msg index 559715faf01a..0c60744d1685 100644 --- a/dom/bindings/Errors.msg +++ b/dom/bindings/Errors.msg @@ -63,4 +63,5 @@ MSG_DEF(MSG_MISSING_REQUIRED_DICTIONARY_MEMBER, 1, "Missing required {0}.") MSG_DEF(MSG_INVALID_REQUEST_METHOD, 1, "Invalid request method {0}.") MSG_DEF(MSG_REQUEST_BODY_CONSUMED_ERROR, 0, "Request body has already been consumed.") MSG_DEF(MSG_RESPONSE_INVALID_STATUSTEXT_ERROR, 0, "Response statusText may not contain newline or carriage return.") +MSG_DEF(MSG_FETCH_FAILED, 0, "NetworkError when attempting to fetch resource.") MSG_DEF(MSG_DEFINE_NON_CONFIGURABLE_PROP_ON_WINDOW, 0, "Not allowed to define a non-configurable property on the WindowProxy object") diff --git a/dom/bindings/Exceptions.cpp b/dom/bindings/Exceptions.cpp index 1352881fa5e3..01fbeaf31bd1 100644 --- a/dom/bindings/Exceptions.cpp +++ b/dom/bindings/Exceptions.cpp @@ -186,6 +186,18 @@ GetCurrentJSStack() return stack.forget(); } +AutoForceSetExceptionOnContext::AutoForceSetExceptionOnContext(JSContext* aCx) + : mCx(aCx) +{ + mOldValue = JS::ContextOptionsRef(mCx).autoJSAPIOwnsErrorReporting(); + JS::ContextOptionsRef(mCx).setAutoJSAPIOwnsErrorReporting(true); +} + +AutoForceSetExceptionOnContext::~AutoForceSetExceptionOnContext() +{ + JS::ContextOptionsRef(mCx).setAutoJSAPIOwnsErrorReporting(mOldValue); +} + namespace exceptions { class StackFrame : public nsIStackFrame diff --git a/dom/bindings/Exceptions.h b/dom/bindings/Exceptions.h index a476b7dd4375..110f060d259d 100644 --- a/dom/bindings/Exceptions.h +++ b/dom/bindings/Exceptions.h @@ -44,6 +44,19 @@ CreateException(JSContext* aCx, nsresult aRv, const char* aMessage = nullptr); already_AddRefed GetCurrentJSStack(); +// Throwing a TypeError on an ErrorResult may result in SpiderMonkey using its +// own error reporting mechanism instead of just setting the exception on the +// context. This happens if no script is running. Bug 1107777 adds a flag that +// forcibly turns this behaviour off. This is a stack helper to set the flag. +class MOZ_STACK_CLASS AutoForceSetExceptionOnContext { +private: + JSContext* mCx; + bool mOldValue; +public: + explicit AutoForceSetExceptionOnContext(JSContext* aCx); + ~AutoForceSetExceptionOnContext(); +}; + // Internal stuff not intended to be widely used. namespace exceptions { diff --git a/dom/bindings/ToJSValue.cpp b/dom/bindings/ToJSValue.cpp index cbefc0218f09..9911d95fefae 100644 --- a/dom/bindings/ToJSValue.cpp +++ b/dom/bindings/ToJSValue.cpp @@ -66,6 +66,7 @@ ToJSValue(JSContext* aCx, JS::MutableHandle aValue) { MOZ_ASSERT(aArgument.Failed()); + AutoForceSetExceptionOnContext forceExn(aCx); DebugOnly throwResult = ThrowMethodFailedWithDetails(aCx, aArgument, "", ""); MOZ_ASSERT(!throwResult); DebugOnly getPendingResult = JS_GetPendingException(aCx, aValue); diff --git a/dom/fetch/Fetch.cpp b/dom/fetch/Fetch.cpp index ce8fc150a235..73d7641e6880 100644 --- a/dom/fetch/Fetch.cpp +++ b/dom/fetch/Fetch.cpp @@ -266,9 +266,15 @@ MainThreadFetchResolver::OnResponseAvailable(InternalResponse* aResponse) NS_ASSERT_OWNINGTHREAD(MainThreadFetchResolver); AssertIsOnMainThread(); - nsCOMPtr go = mPromise->GetParentObject(); - mResponse = new Response(go, aResponse); - mPromise->MaybeResolve(mResponse); + if (aResponse->Type() != ResponseType::Error) { + nsCOMPtr go = mPromise->GetParentObject(); + mResponse = new Response(go, aResponse); + mPromise->MaybeResolve(mResponse); + } else { + ErrorResult result; + result.ThrowTypeError(MSG_FETCH_FAILED); + mPromise->MaybeReject(result); + } } MainThreadFetchResolver::~MainThreadFetchResolver() @@ -296,11 +302,18 @@ public: aWorkerPrivate->AssertIsOnWorkerThread(); MOZ_ASSERT(aWorkerPrivate == mResolver->GetWorkerPrivate()); - nsRefPtr global = aWorkerPrivate->GlobalScope(); - mResolver->mResponse = new Response(global, mInternalResponse); - nsRefPtr promise = mResolver->mFetchPromise.forget(); - promise->MaybeResolve(mResolver->mResponse); + + if (mInternalResponse->Type() != ResponseType::Error) { + nsRefPtr global = aWorkerPrivate->GlobalScope(); + mResolver->mResponse = new Response(global, mInternalResponse); + + promise->MaybeResolve(mResolver->mResponse); + } else { + ErrorResult result; + result.ThrowTypeError(MSG_FETCH_FAILED); + promise->MaybeReject(result); + } return true; } @@ -322,7 +335,6 @@ public: MOZ_ASSERT(aWorkerPrivate); aWorkerPrivate->AssertIsOnWorkerThread(); MOZ_ASSERT(aWorkerPrivate == mResolver->GetWorkerPrivate()); - MOZ_ASSERT(mResolver->mResponse); mResolver->CleanUp(aCx); return true; diff --git a/dom/tests/mochitest/fetch/worker_test_fetch_basic.js b/dom/tests/mochitest/fetch/worker_test_fetch_basic.js index cb82aa80ef6a..28b14d5c3985 100644 --- a/dom/tests/mochitest/fetch/worker_test_fetch_basic.js +++ b/dom/tests/mochitest/fetch/worker_test_fetch_basic.js @@ -23,7 +23,9 @@ function testAboutURL() { }); var p2 = fetch('about:config').then(function(res) { - is(res.type, "error", "about:config should fail"); + ok(false, "about:config should fail"); + }, function(e) { + ok(e instanceof TypeError, "about:config should fail"); }); return Promise.all([p1, p2]); diff --git a/dom/tests/mochitest/fetch/worker_test_fetch_basic_http.js b/dom/tests/mochitest/fetch/worker_test_fetch_basic_http.js index fc9249dba70a..6c706f048729 100644 --- a/dom/tests/mochitest/fetch/worker_test_fetch_basic_http.js +++ b/dom/tests/mochitest/fetch/worker_test_fetch_basic_http.js @@ -43,8 +43,9 @@ function testURLFail() { var promises = []; failFiles.forEach(function(entry) { var p = fetch(entry[0]).then(function(res) { - ok(res.type === "error", "Response should be an error for " + entry[0]); - is(res.status, 0, "Response status should be 0 for " + entry[0]); + ok(false, "Response should be an error for " + entry[0]); + }, function(e) { + ok(e instanceof TypeError, "Response should be an error for " + entry[0]); }); promises.push(p); }); diff --git a/dom/tests/mochitest/fetch/worker_test_fetch_cors.js b/dom/tests/mochitest/fetch/worker_test_fetch_cors.js index 263f6993b373..66ad3f3facc6 100644 --- a/dom/tests/mochitest/fetch/worker_test_fetch_cors.js +++ b/dom/tests/mochitest/fetch/worker_test_fetch_cors.js @@ -24,7 +24,9 @@ function testModeSameOrigin() { // Fetch spec Section 4, step 4, "request's mode is same-origin". var req = new Request("http://example.com", { mode: "same-origin" }); return fetch(req).then(function(res) { - ok(isNetworkError(res), "Attempting to fetch a resource from a different origin with mode same-origin should fail."); + ok(false, "Attempting to fetch a resource from a different origin with mode same-origin should fail."); + }, function(e) { + ok(e instanceof TypeError, "Attempting to fetch a resource from a different origin with mode same-origin should fail."); }); } @@ -660,62 +662,43 @@ function testModeCors() { headers: req.headers, body: req.body }); fetches.push((function(test, request) { return fetch(request).then(function(res) { - dump("Response for " + request.url + "\n"); - if (test.pass) { - ok(!isNetworkError(res), - "shouldn't have failed in test for " + test.toSource()); - if (test.status) { - is(res.status, test.status, "wrong status in test for " + test.toSource()); - is(res.statusText, test.statusMessage, "wrong status text for " + test.toSource()); - } - else { - is(res.status, 200, "wrong status in test for " + test.toSource()); - is(res.statusText, "OK", "wrong status text for " + test.toSource()); - } - if (test.responseHeaders) { - for (header in test.responseHeaders) { - if (test.expectedResponseHeaders.indexOf(header) == -1) { - is(res.headers.has(header), false, - "|Headers.has()|wrong response header (" + header + ") in test for " + - test.toSource()); - } - else { - is(res.headers.get(header), test.responseHeaders[header], - "|Headers.get()|wrong response header (" + header + ") in test for " + - test.toSource()); - } - } - } - - return res.text().then(function(v) { - if (test.method !== "HEAD") { - is(v, "hello pass\n", - "wrong responseText in test for " + test.toSource()); - } - else { - is(v, "", - "wrong responseText in HEAD test for " + test.toSource()); - } - }); + ok(test.pass, "Expected test to pass for " + test.toSource()); + if (test.status) { + is(res.status, test.status, "wrong status in test for " + test.toSource()); + is(res.statusText, test.statusMessage, "wrong status text for " + test.toSource()); } else { - ok(isNetworkError(res), - "should have failed in test for " + test.toSource()); - is(res.status, 0, "wrong status in test for " + test.toSource()); - is(res.statusText, "", "wrong status text for " + test.toSource()); - if (test.responseHeaders) { - for (header in test.responseHeaders) { - is(res.headers.get(header), null, - "wrong response header (" + header + ") in test for " + + is(res.status, 200, "wrong status in test for " + test.toSource()); + is(res.statusText, "OK", "wrong status text for " + test.toSource()); + } + if (test.responseHeaders) { + for (header in test.responseHeaders) { + if (test.expectedResponseHeaders.indexOf(header) == -1) { + is(res.headers.has(header), false, + "|Headers.has()|wrong response header (" + header + ") in test for " + + test.toSource()); + } + else { + is(res.headers.get(header), test.responseHeaders[header], + "|Headers.get()|wrong response header (" + header + ") in test for " + test.toSource()); } } - - return res.text().then(function(v) { - is(v, "", - "wrong responseText in test for " + test.toSource()); - }); } + + return res.text().then(function(v) { + if (test.method !== "HEAD") { + is(v, "hello pass\n", + "wrong responseText in test for " + test.toSource()); + } + else { + is(v, "", + "wrong responseText in HEAD test for " + test.toSource()); + } + }); + }, function(e) { + ok(!test.pass, "Expected test failure for " + test.toSource()); + ok(e instanceof TypeError, "Exception should be TypeError for " + test.toSource()); }); })(test, request)); } @@ -836,24 +819,13 @@ function testCredentials() { } function testResponse(res, test) { - if (test.pass) { - is(isNetworkError(res), false, - "shouldn't have failed in test for " + test.toSource()); - is(res.status, 200, "wrong status in test for " + test.toSource()); - is(res.statusText, "OK", "wrong status text for " + test.toSource()); - return res.text().then(function(v) { - is(v, "hello pass\n", - "wrong text in test for " + test.toSource()); - }); - } - else { - is(isNetworkError(res), true, - "should have failed in test for " + test.toSource()); - return res.text().then(function(v) { - is(v, "", - "wrong text in test for " + test.toSource()); - }); - } + ok(test.pass, "Expected test to pass for " + test.toSource()); + is(res.status, 200, "wrong status in test for " + test.toSource()); + is(res.statusText, "OK", "wrong status text for " + test.toSource()); + return res.text().then(function(v) { + is(v, "hello pass\n", + "wrong text in test for " + test.toSource()); + }); } function runATest(i) { @@ -866,7 +838,15 @@ function testCredentials() { } else { finalPromiseResolve(); } - }, finalPromiseReject); + }, function(e) { + ok(!test.pass, "Expected test failure for " + test.toSource()); + ok(e instanceof TypeError, "Exception should be TypeError for " + test.toSource()); + if (i < tests.length-1) { + runATest(i+1); + } else { + finalPromiseResolve(); + } + }); } runATest(0); @@ -1153,27 +1133,19 @@ function testRedirects() { body: req.body }); fetches.push((function(request, test) { return fetch(request).then(function(res) { - if (test.pass) { - is(isNetworkError(res), false, - "shouldn't have failed in test for " + test.toSource()); - is(res.status, 200, "wrong status in test for " + test.toSource()); - is(res.statusText, "OK", "wrong status text for " + test.toSource()); - is((new URL(res.url)).host, (new URL(test.hops[test.hops.length-1].server)).host, "Response URL should be redirected URL"); - return res.text().then(function(v) { - is(v, "hello pass\n", - "wrong responseText in test for " + test.toSource()); - }); - } - else { - is(isNetworkError(res), true, - "should have failed in test for " + test.toSource()); - is(res.status, 0, "wrong status in test for " + test.toSource()); - is(res.statusText, "", "wrong status text for " + test.toSource()); - return res.text().then(function(v) { - is(v, "", - "wrong responseText in test for " + test.toSource()); - }); - } + ok(test.pass, "Expected test to pass for " + test.toSource()); + is(isNetworkError(res), false, + "shouldn't have failed in test for " + test.toSource()); + is(res.status, 200, "wrong status in test for " + test.toSource()); + is(res.statusText, "OK", "wrong status text for " + test.toSource()); + is((new URL(res.url)).host, (new URL(test.hops[test.hops.length-1].server)).host, "Response URL should be redirected URL"); + return res.text().then(function(v) { + is(v, "hello pass\n", + "wrong responseText in test for " + test.toSource()); + }); + }, function(e) { + ok(!test.pass, "Expected test failure for " + test.toSource()); + ok(e instanceof TypeError, "Exception should be TypeError for " + test.toSource()); }); })(request, test)); }