From 30909c0d26685ed38088cda6e6f75578c8138153 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 20 Nov 2015 16:29:40 -0500 Subject: [PATCH] Bug 1224007 part 1. Rename ThrowMethodFailed to MaybeSetPendingException and make it an ErrorResult instance method. r=peterv --- dom/bindings/BindingUtils.cpp | 71 +++++++++++++++++------------------ dom/bindings/BindingUtils.h | 3 -- dom/bindings/Codegen.py | 39 ++++++++----------- dom/bindings/ErrorResult.h | 38 +++++++++++++++++++ dom/bindings/ToJSValue.cpp | 4 +- dom/cache/CacheStorage.cpp | 4 +- js/xpconnect/src/Sandbox.cpp | 4 +- 7 files changed, 95 insertions(+), 68 deletions(-) diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp index 4002812dc81..d7fe60a80f0 100644 --- a/dom/bindings/BindingUtils.cpp +++ b/dom/bindings/BindingUtils.cpp @@ -125,38 +125,6 @@ ThrowInvalidThis(JSContext* aCx, const JS::CallArgs& aArgs, NamesOfInterfacesWithProtos(aProtoId)); } -bool -ThrowMethodFailed(JSContext* cx, ErrorResult& rv) -{ - if (rv.IsUncatchableException()) { - // Nuke any existing exception on aCx, to make sure we're uncatchable. - JS_ClearPendingException(cx); - // Don't do any reporting. Just return false, to create an - // uncatchable exception. - return false; - } - if (rv.IsJSContextException()) { - // Whatever we need to throw is on the JSContext already. We - // can't assert that there is a pending exception on it, though, - // because in the uncatchable exception case there won't be one. - return false; - } - if (rv.IsErrorWithMessage()) { - rv.ReportErrorWithMessage(cx); - return false; - } - if (rv.IsJSException()) { - rv.ReportJSException(cx); - return false; - } - if (rv.IsDOMException()) { - rv.ReportDOMException(cx); - return false; - } - rv.ReportGenericError(cx); - return false; -} - bool ThrowNoSetterArg(JSContext* aCx, prototypes::ID aProtoId) { @@ -508,6 +476,37 @@ ErrorResult::SuppressException() mResult = NS_OK; } +void +ErrorResult::SetPendingException(JSContext* cx) +{ + if (IsUncatchableException()) { + // Nuke any existing exception on cx, to make sure we're uncatchable. + JS_ClearPendingException(cx); + // Don't do any reporting. Just return, to create an + // uncatchable exception. + return; + } + if (IsJSContextException()) { + // Whatever we need to throw is on the JSContext already. We + // can't assert that there is a pending exception on it, though, + // because in the uncatchable exception case there won't be one. + return; + } + if (IsErrorWithMessage()) { + ReportErrorWithMessage(cx); + return; + } + if (IsJSException()) { + ReportJSException(cx); + return; + } + if (IsDOMException()) { + ReportDOMException(cx); + return; + } + ReportGenericError(cx); +} + namespace dom { bool @@ -2777,10 +2776,10 @@ ConvertExceptionToPromise(JSContext* cx, JS_ClearPendingException(cx); ErrorResult rv; RefPtr promise = Promise::Reject(global, exn, rv); - if (rv.Failed()) { - // We just give up. Make sure to not leak memory on the - // ErrorResult, but then just put the original exception back. - ThrowMethodFailed(cx, rv); + if (rv.MaybeSetPendingException(cx)) { + // We just give up. We put the exception from the ErrorResult on + // the JSContext just to make sure to not leak memory on the + // ErrorResult, but now just put the original exception back. JS_SetPendingException(cx, exn); return false; } diff --git a/dom/bindings/BindingUtils.h b/dom/bindings/BindingUtils.h index 8533180614e..ecb30bffc1f 100644 --- a/dom/bindings/BindingUtils.h +++ b/dom/bindings/BindingUtils.h @@ -92,9 +92,6 @@ ThrowInvalidThis(JSContext* aCx, const JS::CallArgs& aArgs, const ErrNum aErrorNumber, prototypes::ID aProtoId); -bool -ThrowMethodFailed(JSContext* cx, ErrorResult& rv); - // Returns true if the JSClass is used for DOM objects. inline bool IsDOMClass(const JSClass* clasp) diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py index 7e6a7fb912a..8b070e89cf3 100644 --- a/dom/bindings/Codegen.py +++ b/dom/bindings/Codegen.py @@ -1729,8 +1729,7 @@ class CGConstructNavigatorObject(CGAbstractMethod): { // Scope to make sure |result| goes out of scope while |v| is rooted RefPtr result = ConstructNavigatorObjectHelper(aCx, global, rv); rv.WouldReportJSException(); - if (rv.Failed()) { - ThrowMethodFailed(aCx, rv); + if (rv.MaybeSetPendingException(aCx)) { return nullptr; } if (!GetOrCreateDOMReflector(aCx, result, &v)) { @@ -5101,8 +5100,7 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None, } ErrorResult promiseRv; $${declName} = Promise::Resolve(promiseGlobal, $${val}, promiseRv); - if (promiseRv.Failed()) { - ThrowMethodFailed(cx, promiseRv); + if (promiseRv.MaybeSetPendingException(cx)) { $*{exceptionCode} } } @@ -6664,23 +6662,18 @@ class CGCallGenerator(CGThing): A class to generate an actual call to a C++ object. Assumes that the C++ object is stored in a variable whose name is given by the |object| argument. - errorReport should be a CGThing for an error report or None if no - error reporting is needed. + isFallible is a boolean indicating whether the call should be fallible. resultVar: If the returnType is not void, then the result of the call is stored in a C++ variable named by resultVar. The caller is responsible for declaring the result variable. If the caller doesn't care about the result value, resultVar can be omitted. """ - def __init__(self, errorReport, arguments, argsPre, returnType, + def __init__(self, isFallible, arguments, argsPre, returnType, extendedAttributes, descriptorProvider, nativeMethodName, static, object="self", argsPost=[], resultVar=None): CGThing.__init__(self) - assert errorReport is None or isinstance(errorReport, CGThing) - - isFallible = errorReport is not None - result, resultOutParam, resultRooter, resultArgs, resultConversion = \ getRetvalDeclarationForType(returnType, descriptorProvider) @@ -6775,10 +6768,13 @@ class CGCallGenerator(CGThing): if isFallible: self.cgRoot.prepend(CGGeneric("ErrorResult rv;\n")) - self.cgRoot.append(CGGeneric("rv.WouldReportJSException();\n")) - self.cgRoot.append(CGGeneric("if (MOZ_UNLIKELY(rv.Failed())) {\n")) - self.cgRoot.append(CGIndenter(errorReport)) - self.cgRoot.append(CGGeneric("}\n")) + self.cgRoot.append(CGGeneric(dedent( + """ + rv.WouldReportJSException(); + if (MOZ_UNLIKELY(rv.MaybeSetPendingException(cx))) { + return false; + } + """))) self.cgRoot.append(CGGeneric("MOZ_ASSERT(!JS_IsExceptionPending(cx));\n")) @@ -7172,7 +7168,7 @@ class CGPerSignatureCall(CGThing): idlNode.identifier.name)) else: cgThings.append(CGCallGenerator( - self.getErrorReport() if self.isFallible() else None, + self.isFallible(), self.getArguments(), argsPre, returnType, self.extendedAttributes, descriptor, nativeMethodName, static, argsPost=argsPost, resultVar=resultVar)) @@ -7279,9 +7275,6 @@ class CGPerSignatureCall(CGThing): maybeWrap=getMaybeWrapValueFuncForType(self.idlNode.type)) return wrapCode - def getErrorReport(self): - return CGGeneric('return ThrowMethodFailed(cx, rv);\n') - def define(self): return (self.cgRoot.define() + self.wrap_return_value()) @@ -8251,8 +8244,8 @@ class CGEnumerateHook(CGAbstractBindingMethod): ErrorResult rv; self->GetOwnPropertyNames(cx, names, rv); rv.WouldReportJSException(); - if (rv.Failed()) { - return ThrowMethodFailed(cx, rv); + if (rv.MaybeSetPendingException(cx)) { + return false; } bool dummy; for (uint32_t i = 0; i < names.Length(); ++i) { @@ -10309,8 +10302,8 @@ class CGEnumerateOwnPropertiesViaGetOwnPropertyNames(CGAbstractBindingMethod): ErrorResult rv; self->GetOwnPropertyNames(cx, names, rv); rv.WouldReportJSException(); - if (rv.Failed()) { - return ThrowMethodFailed(cx, rv); + if (rv.MaybeSetPendingException(cx)) { + return false; } // OK to pass null as "proxy" because it's ignored if // shadowPrototypeProperties is true diff --git a/dom/bindings/ErrorResult.h b/dom/bindings/ErrorResult.h index 439a39f7061..cf8c949b78f 100644 --- a/dom/bindings/ErrorResult.h +++ b/dom/bindings/ErrorResult.h @@ -134,6 +134,40 @@ public: return rv; } + // Use MaybeSetPendingException to convert an ErrorResult to a pending + // exception on the given JSContext. This is the normal "throw an exception" + // codepath. + // + // The return value is false if the ErrorResult represents success, true + // otherwise. This does mean that in JSAPI method implementations you can't + // just use this as |return rv.MaybeSetPendingException(cx)| (though you could + // |return !rv.MaybeSetPendingException(cx)|), but in practice pretty much any + // consumer would want to do some more work on the success codepath. So + // instead the way you use this is: + // + // if (rv.MaybeSetPendingException(cx)) { + // bail out here + // } + // go on to do something useful + // + // The success path is inline, since it should be the common case and we don't + // want to pay the price of a function call in some of the consumers of this + // method in the common case. + // + // Note that a true return value does NOT mean there is now a pending + // exception on aCx, due to uncatchable exceptions. It should still be + // considered equivalent to a JSAPI failure in terms of what callers should do + // after true is returned. + bool MaybeSetPendingException(JSContext* cx) + { + if (!Failed()) { + return false; + } + + SetPendingException(cx); + return true; + } + template void ThrowTypeError(Ts&&... messageArgs) { @@ -309,6 +343,10 @@ private: // anymore. void ClearUnionData(); + // Implementation of MaybeSetPendingException for the case when we're a + // failure result. + void SetPendingException(JSContext* cx); + // Special values of mResult: // NS_ERROR_TYPE_ERR -- ThrowTypeError() called on us. // NS_ERROR_RANGE_ERR -- ThrowRangeError() called on us. diff --git a/dom/bindings/ToJSValue.cpp b/dom/bindings/ToJSValue.cpp index 108a117993a..8776f55e6d0 100644 --- a/dom/bindings/ToJSValue.cpp +++ b/dom/bindings/ToJSValue.cpp @@ -56,8 +56,8 @@ ToJSValue(JSContext* aCx, MOZ_ASSERT(!aArgument.IsUncatchableException(), "Doesn't make sense to convert uncatchable exception to a JS value!"); AutoForceSetExceptionOnContext forceExn(aCx); - DebugOnly throwResult = ThrowMethodFailed(aCx, aArgument); - MOZ_ASSERT(!throwResult); + DebugOnly throwResult = aArgument.MaybeSetPendingException(aCx); + MOZ_ASSERT(throwResult); DebugOnly getPendingResult = JS_GetPendingException(aCx, aValue); MOZ_ASSERT(getPendingResult); JS_ClearPendingException(aCx); diff --git a/dom/cache/CacheStorage.cpp b/dom/cache/CacheStorage.cpp index 17d16118e6f..d88326e677c 100644 --- a/dom/cache/CacheStorage.cpp +++ b/dom/cache/CacheStorage.cpp @@ -261,8 +261,8 @@ CacheStorage::DefineCaches(JSContext* aCx, JS::Handle aGlobal) false, /* private browsing */ true, /* force trusted */ rv); - if (NS_WARN_IF(rv.Failed())) { - return ThrowMethodFailed(aCx, rv); + if (NS_WARN_IF(rv.MaybeSetPendingException(aCx))) { + return false; } JS::Rooted caches(aCx); diff --git a/js/xpconnect/src/Sandbox.cpp b/js/xpconnect/src/Sandbox.cpp index 4a92b3f6fa8..1e800beb197 100644 --- a/js/xpconnect/src/Sandbox.cpp +++ b/js/xpconnect/src/Sandbox.cpp @@ -290,8 +290,8 @@ SandboxFetch(JSContext* cx, JS::HandleObject scope, const CallArgs& args) RefPtr response = FetchRequest(global, Constify(request), Constify(options), rv); rv.WouldReportJSException(); - if (rv.Failed()) { - return ThrowMethodFailed(cx, rv); + if (rv.MaybeSetPendingException(cx)) { + return false; } if (!GetOrCreateDOMReflector(cx, response, args.rval())) { return false;