From 3e01108efeb98d5cd791bb3b42b2994f09ae5083 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@mit.edu>
Date: Fri, 20 Nov 2015 16:29:41 -0500
Subject: [PATCH] Bug 1224007 part 6.  Change MaybeSetPendingException to set
 the ErrorResult state to "not failed", just like SuppressException and
 StealNSResult already do, and assert in the destructor that the ErrorResult
 is not Failed().

This is not quite as strong as being able to assert that all codepaths that might
lead to failure call one of the above methods, but being able to assert that
would involve a lot of extra noise at callsites.  Or at least changing the
signature of StealNSResult to use an outparam and return a boolean indicating
whether the ErrorResult was failure or not, or something, so StealNSResult
can be usefully called on successful ErrorResults too.
---
 dom/bindings/BindingUtils.cpp |  9 ++++++---
 dom/bindings/ErrorResult.h    | 19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp
index a604210e104..f6d0167aa40 100644
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -209,6 +209,7 @@ ErrorResult::SetPendingExceptionWithMessage(JSContext* aCx)
                               argCount > 0 ? args : nullptr);
 
   ClearMessage();
+  mResult = NS_OK;
 }
 
 void
@@ -263,9 +264,7 @@ ErrorResult::SetPendingJSException(JSContext* cx)
   // what, go ahead and unroot mJSException.
   js::RemoveRawValueRoot(cx, &mJSException);
 
-  // We no longer have a useful exception but we do want to signal that an error
-  // occured.
-  mResult = NS_ERROR_FAILURE;
+  mResult = NS_OK;
 #ifdef DEBUG
   mUnionState = HasNothing;
 #endif // DEBUG
@@ -333,6 +332,7 @@ ErrorResult::SetPendingDOMException(JSContext* cx)
   dom::Throw(cx, mDOMExceptionInfo->mRv, mDOMExceptionInfo->mMessage);
 
   ClearDOMExceptionInfo();
+  mResult = NS_OK;
 }
 
 void
@@ -372,6 +372,7 @@ ErrorResult::SetPendingGenericErrorException(JSContext* cx)
   MOZ_ASSERT(!IsJSException());
   MOZ_ASSERT(!IsDOMException());
   dom::Throw(cx, ErrorCode());
+  mResult = NS_OK;
 }
 
 ErrorResult&
@@ -468,12 +469,14 @@ ErrorResult::SetPendingException(JSContext* cx)
     JS_ClearPendingException(cx);
     // Don't do any reporting.  Just return, to create an
     // uncatchable exception.
+    mResult = NS_OK;
     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.
+    mResult = NS_OK;
     return;
   }
   if (IsErrorWithMessage()) {
diff --git a/dom/bindings/ErrorResult.h b/dom/bindings/ErrorResult.h
index a2c7d45bcc3..6e24b932986 100644
--- a/dom/bindings/ErrorResult.h
+++ b/dom/bindings/ErrorResult.h
@@ -6,6 +6,17 @@
 
 /**
  * A struct for tracking exceptions that need to be thrown to JS.
+ *
+ * Conceptually, an ErrorResult represents either success or an exception in the
+ * process of being thrown.  This means that a failing ErrorResult _must_ be
+ * handled in one of the following ways before coming off the stack:
+ *
+ * 1) Suppressed via SuppressException().
+ * 2) Converted to a pure nsresult return value via StealNSResult().
+ * 3) Converted to an actual pending exception on a JSContext via
+ *    MaybeSetPendingException.
+ * 4) Converted to an exception JS::Value (probably to then reject a Promise
+ *    with) via dom::ToJSValue.
  */
 
 #ifndef mozilla_ErrorResult_h
@@ -87,8 +98,9 @@ public:
 
 #ifdef DEBUG
   ~ErrorResult() {
-    MOZ_ASSERT_IF(IsErrorWithMessage(), !mMessage);
-    MOZ_ASSERT_IF(IsDOMException(), !mDOMExceptionInfo);
+    // Consumers should have called one of MaybeSetPendingException
+    // (possibly via ToJSValue), StealNSResult, and SuppressException
+    MOZ_ASSERT(!Failed());
     MOZ_ASSERT(!mMightHaveUnreportedJSException);
     MOZ_ASSERT(mUnionState == HasNothing);
   }
@@ -158,6 +170,9 @@ public:
   // 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.
+  //
+  // After this call, the ErrorResult will no longer return true from Failed(),
+  // since the exception will have moved to the JSContext.
   bool MaybeSetPendingException(JSContext* cx)
   {
     WouldReportJSException();