[Promises] Fulfill promise before firing PromiseRejectCallback

Previously, we would first fire the PromiseRejectCallback before
fulfilling the promise. This patch changes the behavior to first
fulfill the promise. This behavior is more intuitive.

This patch also merges the check for PromiseHook callback with the
debug callback, since they use the same boolean bit on the isolate.

Bug: v8:6880
Change-Id: Ia04867e16423a1d6006f0f3f93a14fa6026e17ed
Reviewed-on: https://chromium-review.googlesource.com/700980
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48298}

src/builtins/builtins-promise-gen.cc

@@ -959,32 +959,23 @@ void PromiseBuiltinsAssembler::InternalPromiseReject(Node* context,
 void PromiseBuiltinsAssembler::InternalPromiseReject(Node* context,
                                                      Node* promise, Node* value,
                                                      bool debug_event) {
-  Label fulfill(this), report_unhandledpromise(this), run_promise_hook(this);
+  Label fulfill(this), exit(this);
 
+  GotoIfNot(IsPromiseHookEnabledOrDebugIsActive(), &fulfill);
   if (debug_event) {
-    GotoIfNot(IsDebugActive(), &run_promise_hook);
     CallRuntime(Runtime::kDebugPromiseReject, context, promise, value);
-    Goto(&run_promise_hook);
-  } else {
-    Goto(&run_promise_hook);
-  }
-
-  BIND(&run_promise_hook);
-  {
-    GotoIfNot(IsPromiseHookEnabledOrDebugIsActive(), &report_unhandledpromise);
-    CallRuntime(Runtime::kPromiseHookResolve, context, promise);
-    Goto(&report_unhandledpromise);
-  }
-
-  BIND(&report_unhandledpromise);
-  {
-    GotoIf(PromiseHasHandler(promise), &fulfill);
-    CallRuntime(Runtime::kReportPromiseReject, context, promise, value);
-    Goto(&fulfill);
   }
+  CallRuntime(Runtime::kPromiseHookResolve, context, promise);
+  Goto(&fulfill);
 
   BIND(&fulfill);
   PromiseFulfill(context, promise, value, v8::Promise::kRejected);
+
+  GotoIf(PromiseHasHandler(promise), &exit);
+  CallRuntime(Runtime::kReportPromiseReject, context, promise, value);
+  Goto(&exit);
+
+  BIND(&exit);
 }
 
 void PromiseBuiltinsAssembler::SetForwardingHandlerIfTrue(

src/isolate.cc

@@ -3417,10 +3417,10 @@ void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
   promise_reject_callback_ = callback;
 }
 
-
-void Isolate::ReportPromiseReject(Handle<JSObject> promise,
+void Isolate::ReportPromiseReject(Handle<JSPromise> promise,
                                   Handle<Object> value,
                                   v8::PromiseRejectEvent event) {
+  DCHECK_EQ(v8::Promise::kRejected, promise->status());
   if (promise_reject_callback_ == NULL) return;
   Handle<FixedArray> stack_trace;
   if (event == v8::kPromiseRejectWithNoHandler && value->IsJSObject()) {

src/isolate.h

@@ -1173,7 +1173,7 @@ class Isolate {
   inline void FireMicrotasksCompletedCallback();
 
   void SetPromiseRejectCallback(PromiseRejectCallback callback);
-  void ReportPromiseReject(Handle<JSObject> promise, Handle<Object> value,
+  void ReportPromiseReject(Handle<JSPromise> promise, Handle<Object> value,
                            v8::PromiseRejectEvent event);
 
   void PromiseReactionJob(Handle<PromiseReactionJobInfo> info,

src/runtime/runtime-promise.cc

@@ -26,7 +26,7 @@ void PromiseRejectEvent(Isolate* isolate, Handle<JSPromise> promise,
 
   // Report only if we don't actually have a handler.
   if (!promise->has_handler()) {
-    isolate->ReportPromiseReject(Handle<JSObject>::cast(promise), value,
+    isolate->ReportPromiseReject(promise, value,
                                  v8::kPromiseRejectWithNoHandler);
   }
 }
@@ -58,8 +58,7 @@ RUNTIME_FUNCTION(Runtime_ReportPromiseReject) {
   HandleScope scope(isolate);
   CONVERT_ARG_HANDLE_CHECKED(JSPromise, promise, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
-  isolate->ReportPromiseReject(Handle<JSObject>::cast(promise), value,
-                               v8::kPromiseRejectWithNoHandler);
+  isolate->ReportPromiseReject(promise, value, v8::kPromiseRejectWithNoHandler);
   return isolate->heap()->undefined_value();
 }
 

test/cctest/test-api.cc

@@ -17606,6 +17606,8 @@ int promise_reject_frame_count = -1;
 void PromiseRejectCallback(v8::PromiseRejectMessage reject_message) {
   v8::Local<v8::Object> global = CcTest::global();
   v8::Local<v8::Context> context = CcTest::isolate()->GetCurrentContext();
+  CHECK_EQ(v8::Promise::PromiseState::kRejected,
+           reject_message.GetPromise()->State());
   if (reject_message.GetEvent() == v8::kPromiseRejectWithNoHandler) {
     promise_reject_counter++;
     global->Set(context, v8_str("rejected"), reject_message.GetPromise())