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().
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 20 Nov 2015 16:29:41 -0500
changeset 273560 ecb3051bba081b246f8504d23ab568617e9248c5
parent 273559 e597563f32ff62d4fb5ae612cebf93a630b20621
child 273561 bf248ceb561b6faed0d01c4320a824a3ccdf22fb
push id68323
push userbzbarsky@mozilla.com
push dateFri, 20 Nov 2015 21:29:52 +0000
treeherdermozilla-inbound@312f48463ab9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1224007
milestone45.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/bindings/ErrorResult.h
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -204,16 +204,17 @@ ErrorResult::SetPendingExceptionWithMess
   }
   args[argCount] = nullptr;
 
   JS_ReportErrorNumberUCArray(aCx, dom::GetErrorMessage, nullptr,
                               static_cast<const unsigned>(message->mErrorNumber),
                               argCount > 0 ? args : nullptr);
 
   ClearMessage();
+  mResult = NS_OK;
 }
 
 void
 ErrorResult::ClearMessage()
 {
   MOZ_ASSERT(IsErrorWithMessage());
   delete mMessage;
   mMessage = nullptr;
@@ -258,19 +259,17 @@ ErrorResult::SetPendingJSException(JSCon
   if (JS_WrapValue(cx, &exception)) {
     JS_SetPendingException(cx, exception);
   }
   mJSException = exception;
   // If JS_WrapValue failed, not much we can do about it...  No matter
   // 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
 }
 
 struct ErrorResult::DOMExceptionInfo {
   DOMExceptionInfo(nsresult rv, const nsACString& message)
     : mMessage(message)
@@ -328,16 +327,17 @@ ErrorResult::SetPendingDOMException(JSCo
 {
   MOZ_ASSERT(mDOMExceptionInfo,
              "SetPendingDOMException() can be called only once");
   MOZ_ASSERT(mUnionState == HasDOMExceptionInfo);
 
   dom::Throw(cx, mDOMExceptionInfo->mRv, mDOMExceptionInfo->mMessage);
 
   ClearDOMExceptionInfo();
+  mResult = NS_OK;
 }
 
 void
 ErrorResult::ClearDOMExceptionInfo()
 {
   MOZ_ASSERT(IsDOMException());
   MOZ_ASSERT(mUnionState == HasDOMExceptionInfo || !mDOMExceptionInfo);
   delete mDOMExceptionInfo;
@@ -367,16 +367,17 @@ ErrorResult::ClearUnionData()
 
 void
 ErrorResult::SetPendingGenericErrorException(JSContext* cx)
 {
   MOZ_ASSERT(!IsErrorWithMessage());
   MOZ_ASSERT(!IsJSException());
   MOZ_ASSERT(!IsDOMException());
   dom::Throw(cx, ErrorCode());
+  mResult = NS_OK;
 }
 
 ErrorResult&
 ErrorResult::operator=(ErrorResult&& aRHS)
 {
   // Clear out any union members we may have right now, before we
   // start writing to it.
   ClearUnionData();
@@ -463,22 +464,24 @@ ErrorResult::SuppressException()
 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.
+    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()) {
     SetPendingExceptionWithMessage(cx);
     return;
   }
   if (IsJSException()) {
     SetPendingJSException(cx);
--- a/dom/bindings/ErrorResult.h
+++ b/dom/bindings/ErrorResult.h
@@ -1,16 +1,27 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /**
  * 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
 #define mozilla_ErrorResult_h
 
 #include <stdarg.h>
 
 #include "js/Value.h"
@@ -82,18 +93,19 @@ public:
     , mMightHaveUnreportedJSException(false)
     , mUnionState(HasNothing)
 #endif
   {
   }
 
 #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);
   }
 #endif // DEBUG
 
   ErrorResult(ErrorResult&& aRHS)
     // Initialize mResult and whatever else we need to default-initialize, so
     // the ClearUnionData call in our operator= will do the right thing
@@ -153,16 +165,19 @@ public:
   // 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.
+  //
+  // 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();
     if (!Failed()) {
       return false;
     }
 
     SetPendingException(cx);