Issue #2435 - Re-Implement rejected Promises events according to spec

Own implementation; upstream gets several things wrong.

dom/promise/PromiseDebugging.cpp

@@ -217,8 +217,16 @@ PromiseDebugging::RemoveUncaughtRejectionObserver(GlobalObject&,
 /* static */ void
 PromiseDebugging::AddUncaughtRejection(JS::HandleObject aPromise)
 {
+  CycleCollectedJSContext* storage = CycleCollectedJSContext::Get();
+  auto& uncaught = storage->mUncaughtRejections;
+  auto& outstanding = storage->mOutstandingRejections;
+
+  // See 8.1.4.7 Unhandled promise rejections, Step 5.1.4.
+  RefPtr<Promise> promise = Promise::CreateFromExisting(xpc::NativeGlobal(aPromise), aPromise);
+  uint64_t promiseID = JS::GetPromiseID(aPromise);
+  outstanding.Put(promiseID, promise);
   // This might OOM, but won't set a pending exception, so we'll just ignore it.
-  if (CycleCollectedJSContext::Get()->mUncaughtRejections.append(aPromise)) {
+  if (uncaught.append(aPromise)) {
     FlushRejections::DispatchNeeded();
   }
 }
@@ -226,15 +234,20 @@ PromiseDebugging::AddUncaughtRejection(JS::HandleObject aPromise)
 /* void */ void
 PromiseDebugging::AddConsumedRejection(JS::HandleObject aPromise)
 {
+  CycleCollectedJSContext* storage = CycleCollectedJSContext::Get();
+  auto& uncaught = storage->mUncaughtRejections;
+  auto& outstanding = storage->mOutstandingRejections;
+
   // If the promise is in our list of uncaught rejections, we haven't yet
   // reported it as unhandled. In that case, just remove it from the list
   // and don't add it to the list of consumed rejections.
-  auto& uncaughtRejections = CycleCollectedJSContext::Get()->mUncaughtRejections;
+  uint64_t promiseID = JS::GetPromiseID(aPromise);
-  for (size_t i = 0; i < uncaughtRejections.length(); i++) {
+  outstanding.Remove(promiseID);
-    if (uncaughtRejections[i] == aPromise) {
+  for (size_t i = 0; i < uncaught.length(); i++) {
+    if (uncaught[i] == aPromise) {
       // To avoid large amounts of memmoves, we don't shrink the vector here.
       // Instead, we filter out nullptrs when iterating over the vector later.
-      uncaughtRejections[i].set(nullptr);
+      uncaught[i].set(nullptr);
       return;
     }
   }
@@ -251,6 +264,7 @@ PromiseDebugging::FlushUncaughtRejectionsInternal()
 
   auto& uncaught = storage->mUncaughtRejections;
   auto& consumed = storage->mConsumedRejections;
+  auto& outstanding = storage->mOutstandingRejections;
 
   AutoJSAPI jsapi;
   jsapi.Init();
@@ -267,6 +281,10 @@ PromiseDebugging::FlushUncaughtRejectionsInternal()
       continue;
     }
 
+    // Clean up outstanding rejected promises weak set
+    uint64_t promiseID = JS::GetPromiseID(promise);
+    outstanding.Remove(promiseID);
+
     for (size_t j = 0; j < observers.Length(); ++j) {
       RefPtr<UncaughtRejectionObserver> obs =
         static_cast<UncaughtRejectionObserver*>(observers[j].get());
@@ -274,8 +292,12 @@ PromiseDebugging::FlushUncaughtRejectionsInternal()
       IgnoredErrorResult err;
       obs->OnLeftUncaught(promise, err);
     }
-    JSAutoCompartment ac(cx, promise);
+    // report error to console, unless marked by unhandledrejection event
-    Promise::ReportRejectedPromise(cx, promise);
+    bool reportError = !JS::GetPromiseIsReported(promise);
+    if (reportError) {
+      JSAutoCompartment ac(cx, promise);
+      Promise::ReportRejectedPromise(cx, promise);
+    }
   }
   storage->mUncaughtRejections.clear();
 

js/src/builtin/Promise.h

@@ -100,6 +100,10 @@ class PromiseObject : public NativeObject
         int32_t flags = getFixedSlot(PromiseSlot_Flags).toInt32();
         setFixedSlot(PromiseSlot_Flags, Int32Value(flags | PROMISE_FLAG_REPORTED));
     }
+    bool isReported() {
+        MOZ_ASSERT(isUnhandled());
+        return flags() & PROMISE_FLAG_REPORTED;
+    }
 };
 
 /**

js/src/jsapi.cpp

@@ -4962,6 +4962,18 @@ JS::GetPromiseAllocationSite(JS::HandleObject promise)
     return promise->as<PromiseObject>().allocationSite();
 }
 
+JS_PUBLIC_API(bool)
+JS::GetPromiseIsReported(JS::HandleObject promise)
+{
+    return promise->as<PromiseObject>().isReported();
+}
+
+JS_PUBLIC_API(void)
+JS::MarkPromiseRejectionReported(JS::HandleObject promise)
+{
+    return promise->as<PromiseObject>().markAsReported();
+}
+
 JS_PUBLIC_API(JSObject*)
 JS::GetPromiseResolutionSite(JS::HandleObject promise)
 {

js/src/jsapi.h

@@ -4620,6 +4620,18 @@ GetPromiseResult(JS::HandleObject promise);
 extern JS_PUBLIC_API(bool)
 GetPromiseIsHandled(JS::HandleObject promise);
 
+/**
+ * Returns whether the given promise's rejection is reported or not.
+ */
+extern JS_PUBLIC_API(bool)
+GetPromiseIsReported(JS::HandleObject promise);
+
+/**
+ * Mark the given promise's rejection as already reported.
+ */
+extern JS_PUBLIC_API(void)
+MarkPromiseRejectionReported(JS::HandleObject promise);
+
 /**
  * Returns a js::SavedFrame linked list of the stack that lead to the given
  * Promise's allocation.

xpcom/base/CycleCollectedJSContext.cpp

@@ -1005,17 +1005,24 @@ CycleCollectedJSContext::PromiseRejectionTrackerCallback(JSContext* aCx,
   // TODO: Bug 1549351 - Promise rejection event should not be sent for
   // cross-origin scripts
 
+
+  // See HTML 8.1.6.3 HostPromiseRejectionTracker
+
   PromiseArray& aboutToBeNotified = self->mAboutToBeNotifiedRejectedPromises;
-  PromiseHashtable& unhandled = self->mPendingUnhandledRejections;
+  PromiseHashtable& outstanding = self->mOutstandingRejections;
   uint64_t promiseID = JS::GetPromiseID(aPromise);
 
   if (state == PromiseRejectionHandlingState::Unhandled) {
-    PromiseDebugging::AddUncaughtRejection(aPromise);
+    // Step 5.
+    // Unhandled Promises first go to the about-to-be-notified queue, processed
+    // after Microtasks as NotifyUnhandledRejections
     RefPtr<Promise> promise = Promise::CreateFromExisting(xpc::NativeGlobal(aPromise), aPromise);
     aboutToBeNotified.AppendElement(promise);
-    unhandled.Put(promiseID, promise);
   } else {
-    PromiseDebugging::AddConsumedRejection(aPromise);
+    // Unhandled-then-handled Promises (ie.e Promises where the unhandledrejection event
+    // handler attached a handler) may fire the rejectionhandled event.
+
+    // Step 6.1.
     for (size_t i = 0; i < aboutToBeNotified.Length(); i++) {
       if (aboutToBeNotified[i] &&
           aboutToBeNotified[i]->PromiseObj() == aPromise) {
@@ -1023,16 +1030,19 @@ CycleCollectedJSContext::PromiseRejectionTrackerCallback(JSContext* aCx,
         // here. Instead, we filter out nullptrs when iterating over the
         // vector later.
         aboutToBeNotified[i] = nullptr;
-        MOZ_ASSERT(unhandled.Get(promiseID, nullptr));
+        MOZ_ASSERT(!outstanding.Get(promiseID, nullptr));
-        unhandled.Remove(promiseID);
         return;
       }
     }
+    // Step 6.2. and 6.3.
     RefPtr<Promise> promise;
-    unhandled.Remove(promiseID, getter_AddRefs(promise));
+    if (outstanding.Remove(promiseID, getter_AddRefs(promise))) {
-    if (!promise) {
+      // Step 6.4.
       nsIGlobalObject* global = xpc::NativeGlobal(aPromise);
       if (nsCOMPtr<EventTarget> owner = do_QueryInterface(global)) {
+        // Step 6.5.
+        // The spec says to do this in a global task, but we're just queuing an event,
+        // which will execute in the next event loop either way.
         PromiseRejectionEventInit init;
         init.mPromise = Promise::CreateFromExisting(global, aPromise);
         init.mReason = JS::GetPromiseResult(aPromise);
@@ -1044,6 +1054,9 @@ CycleCollectedJSContext::PromiseRejectionTrackerCallback(JSContext* aCx,
         asyncDispatcher->PostDOMEvent();
       }
     }
+
+    // Finally, notifiy debug observers
+    PromiseDebugging::AddConsumedRejection(aPromise);
   }
 }
 
@@ -1833,7 +1846,11 @@ CycleCollectedJSContext::PerformDebuggerMicroTaskCheckpoint()
 NS_IMETHODIMP
 CycleCollectedJSContext::NotifyUnhandledRejections::Run()
 {
+  // See HTML 8.1.4.7 Unhandled promise rejections
+  // with 'mUnhandledRejections' == copy of 'about-to-be-notified rejected'
+
   for (size_t i = 0; i < mUnhandledRejections.Length(); ++i) {
+    // Step 5.1.
     RefPtr<Promise>& promise = mUnhandledRejections[i];
     if (!promise)
       continue;
@@ -1841,35 +1858,40 @@ CycleCollectedJSContext::NotifyUnhandledRejections::Run()
     JS::RootedObject promiseObj(mCx->RootingCx(), promise->PromiseObj());
     MOZ_ASSERT(JS::IsPromiseObject(promiseObj));
 
-    // Only fire unhandledrejection if the promise is still not handled;
+    // Step 5.1.1.
+    if (JS::GetPromiseIsHandled(promiseObj))
+      continue;
+
     uint64_t promiseID = JS::GetPromiseID(promiseObj);
-    if (!JS::GetPromiseIsHandled(promiseObj)) {
-      if (nsCOMPtr<EventTarget> target =
-              do_QueryInterface(promise->GetParentObject())) {
-        PromiseRejectionEventInit init;
-        init.mPromise = promise;
-        init.mReason = JS::GetPromiseResult(promiseObj);
-        init.mCancelable = true;
 
-        RefPtr<PromiseRejectionEvent> event =
+    // Step 5.1.2.
-            PromiseRejectionEvent::Constructor(
+    bool raiseError = true;
-                target, NS_LITERAL_STRING("unhandledrejection"), init);
+    if (nsCOMPtr<EventTarget> target =
-        // We don't use the result of dispatching event here to check whether to
+            do_QueryInterface(promise->GetParentObject())) {
-        // report the Promise to console.
+      PromiseRejectionEventInit init;
-        bool dummy = true;
+      init.mPromise = promise;
-        target->DispatchEvent(event, &dummy);
+      init.mReason = JS::GetPromiseResult(promiseObj);
-      }
+      init.mCancelable = true;
+
+      RefPtr<PromiseRejectionEvent> event =
+          PromiseRejectionEvent::Constructor(
+              target, NS_LITERAL_STRING("unhandledrejection"), init);
+
+      // Step 5.1.4. (reordered)
+      // In contrast to the letter of the spec, this must be done before
+      // the event handler is called or the PromiseRejectionTrackerCallback
+      // will not see the correct state.
+      PromiseDebugging::AddUncaughtRejection(promiseObj);
+      target->DispatchEvent(event, &raiseError);
     }
 
-    if (!JS::GetPromiseIsHandled(promiseObj)) {
+    // Step 5.1.3. (implied)
-      MOZ_ASSERT(mCx->mPendingUnhandledRejections.Get(promiseID, nullptr));
+    // If the Promise became handled, PromiseRejectionTrackerCallback will have marked it.
-      mCx->mPendingUnhandledRejections.Remove(promiseID);
+    // If not, keep it in the "uncaught" list for PromiseDebugging, and set the flag if
+    // preventDefault indicated we don't want it reported to console.
+    if (!JS::GetPromiseIsHandled(promiseObj) && !raiseError) {
+      JS::MarkPromiseRejectionReported(promiseObj);
     }
-
-    // If a rejected promise is being handled in "unhandledrejection" event
-    // handler, it should be removed from the table in
-    // PromiseRejectionTrackerCallback.
-    MOZ_ASSERT(!mCx->mPendingUnhandledRejections.Get(promiseID, nullptr));
   }
   return NS_OK;
 }
@@ -1883,7 +1905,7 @@ CycleCollectedJSContext::NotifyUnhandledRejections::Cancel()
       continue;
 
     JS::RootedObject promiseObj(mCx->RootingCx(), promise->PromiseObj());
-    mCx->mPendingUnhandledRejections.Remove(JS::GetPromiseID(promiseObj));
+    mCx->mOutstandingRejections.Remove(JS::GetPromiseID(promiseObj));
   }
   return NS_OK;
 }

xpcom/base/CycleCollectedJSContext.h

@@ -430,6 +430,12 @@ public:
   // in the last turn of the event loop.
   JS::PersistentRooted<JS::GCVector<JSObject*, 0, js::SystemAllocPolicy>> mConsumedRejections;
 
+  // This is for the "outstanding rejected promises weak set" in the spec,
+  // https://html.spec.whatwg.org/multipage/webappapis.html#outstanding-rejected-promises-weak-set
+  typedef nsRefPtrHashtable<nsUint64HashKey, dom::Promise> PromiseHashtable;
+  PromiseHashtable mOutstandingRejections;
+
+
   nsTArray<nsCOMPtr<nsISupports /* UncaughtRejectionObserver */ >> mUncaughtRejectionObservers;
 
 private:
@@ -480,25 +486,6 @@ private:
   typedef nsTArray<RefPtr<dom::Promise>> PromiseArray;
   PromiseArray mAboutToBeNotifiedRejectedPromises;
 
-  // This is for the "outstanding rejected promises weak set" in the spec,
-  // https://html.spec.whatwg.org/multipage/webappapis.html#outstanding-rejected-promises-weak-set
-  // We use different data structure and opposite logic here to achieve the same
-  // effect. Basically this is used for tracking the rejected promise that does
-  // NOT need firing a rejectionhandled event. We will check the table to see if
-  // firing rejectionhandled event is required when a rejected promise is being
-  // handled.
-  //
-  // The rejected promise will be stored in the table if
-  // - it is unhandled, and
-  // - The unhandledrejection is not yet fired.
-  //
-  // And be removed when
-  // - it is handled, or
-  // - A unhandledrejection is fired and it isn't being handled in event
-  // handler.
-  typedef nsRefPtrHashtable<nsUint64HashKey, dom::Promise> PromiseHashtable;
-  PromiseHashtable mPendingUnhandledRejections;
-
   class NotifyUnhandledRejections final : public CancelableRunnable {
    public:
     NotifyUnhandledRejections(CycleCollectedJSContext* aCx,