Bug 1224007 part 1. Rename ThrowMethodFailed to MaybeSetPendingException and make it an ErrorResult instance method. r=peterv

Author: bzbarsky

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 = 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;
   }

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)

dom/bindings/Codegen.py

@@ -1729,8 +1729,7 @@ def definition_body(self):
             {  // Scope to make sure |result| goes out of scope while |v| is rooted
               RefPtr<mozilla::dom::${descriptorName}> 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 handleNull(templateBody, setToNullVar, extraConditionForNull=""):
                   }
                   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 @@ def needsConst(a):
 
         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 @@ def __init__(self, returnType, arguments, nativeMethodName, static,
                                                           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 @@ def wrap_return_value(self):
                 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 @@ def generate_code(self):
             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 @@ def generate_code(self):
             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

dom/bindings/ErrorResult.h

@@ -134,6 +134,40 @@ class ErrorResult {
     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<dom::ErrNum errorNumber, typename... Ts>
   void ThrowTypeError(Ts&&... messageArgs)
   {
@@ -309,6 +343,10 @@ class ErrorResult {
   // 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.

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<bool> throwResult = ThrowMethodFailed(aCx, aArgument);
-  MOZ_ASSERT(!throwResult);
+  DebugOnly<bool> throwResult = aArgument.MaybeSetPendingException(aCx);
+  MOZ_ASSERT(throwResult);
   DebugOnly<bool> getPendingResult = JS_GetPendingException(aCx, aValue);
   MOZ_ASSERT(getPendingResult);
   JS_ClearPendingException(aCx);

dom/cache/CacheStorage.cpp

@@ -261,8 +261,8 @@ CacheStorage::DefineCaches(JSContext* aCx, JS::Handle<JSObject*> 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<JS::Value> caches(aCx);

js/xpconnect/src/Sandbox.cpp

@@ -290,8 +290,8 @@ SandboxFetch(JSContext* cx, JS::HandleObject scope, const CallArgs& args)
     RefPtr<dom::Promise> 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;