[gjs: 1/3] context: Only warn about unhandled promises when the job queue is flushed

[Philip Chimento <pchimento src gnome org>]

commit 3f0c615a802e196f5e14a9819a81e7c74fb6b988
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Mon Feb 21 17:17:10 2022 +0100

    context: Only warn about unhandled promises when the job queue is flushed
    
    A promise rejection may be reported as not handled even though it's
    going to be handled by a later added promise. In such case we were
    correctly be informed about this via
    unregister_unhandled_promise_rejection(), but given that since commit
    4446ee7f we promptly warn about unhandled promise rejections our
    unhandled rejections stack trace list was cleared earlier than we'd got
    an unregistration event.
    
    To avoid this, let's just wait for the job queue to be fully processed,
    so that we won't accidentally clear the unhandled rejections list, until
    those have been fully processed.
    
    By doing this we are both avoiding misbehaving on the said case, and we
    don't regress by keeping the exit-warns handled by #417 still valid.
    
    Added tests to cover both a normal rejection case and the one that was
    previously leading to a crash (assertion error).
    
    Fixes: #466

 gi/closure.cpp                    |   4 +-
 gi/function.cpp                   |   6 +--
 gjs/context.cpp                   |   6 +--
 installed-tests/js/meson.build    |   1 +
 installed-tests/js/testPromise.js | 106 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 115 insertions(+), 8 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 15b6a5fe7..288550a05 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -183,7 +183,9 @@ bool Closure::invoke(JS::HandleObject this_obj,
     JS::RootedFunction func(m_cx, m_func);
     bool ok = JS::Call(m_cx, this_obj, func, args, retval);
     GjsContextPrivate* gjs = GjsContextPrivate::from_cx(m_cx);
-    gjs->warn_about_unhandled_promise_rejections();
+
+    if (gjs->should_exit(nullptr))
+        gjs->warn_about_unhandled_promise_rejections();
     if (!ok) {
         /* Exception thrown... */
         gjs_debug_closure(
diff --git a/gi/function.cpp b/gi/function.cpp
index f39dc1727..716acc8ec 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -379,11 +379,11 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
             // main loop, or maybe not, but there's no way to tell, so we have
             // to exit here instead of propagating the exception back to the
             // original calling JS code.
-            gjs->warn_about_unhandled_promise_rejections();
-
             uint8_t code;
-            if (gjs->should_exit(&code))
+            if (gjs->should_exit(&code)) {
+                gjs->warn_about_unhandled_promise_rejections();
                 exit(code);
+            }
 
             // Some other uncatchable exception, e.g. out of memory
             JSFunction* fn = callable();
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 7a6eba6e7..0ba6b250a 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -1080,10 +1080,7 @@ bool GjsContextPrivate::run_jobs_fallible(GCancellable* cancellable) {
             JSAutoRealm ar(m_cx, job);
             gjs_debug(GJS_DEBUG_CONTEXT, "handling job %s",
                       gjs_debug_object(job).c_str());
-            bool ok =
-                JS::Call(m_cx, JS::UndefinedHandleValue, job, args, &rval);
-            warn_about_unhandled_promise_rejections();
-            if (!ok) {
+            if (!JS::Call(m_cx, JS::UndefinedHandleValue, job, args, &rval)) {
                 /* Uncatchable exception - return false so that
                  * System.exit() works in the interactive shell and when
                  * exiting the interpreter. */
@@ -1104,6 +1101,7 @@ bool GjsContextPrivate::run_jobs_fallible(GCancellable* cancellable) {
 
     m_draining_job_queue = false;
     m_job_queue.clear();
+    warn_about_unhandled_promise_rejections();
     JS::JobQueueIsEmpty(m_cx);
     return retval;
 }
diff --git a/installed-tests/js/meson.build b/installed-tests/js/meson.build
index f531fd356..567e5cc7c 100644
--- a/installed-tests/js/meson.build
+++ b/installed-tests/js/meson.build
@@ -137,6 +137,7 @@ jasmine_tests = [
     'Package',
     'ParamSpec',
     'Print',
+    'Promise',
     'Regress',
     'Signals',
     'System',
diff --git a/installed-tests/js/testPromise.js b/installed-tests/js/testPromise.js
new file mode 100644
index 000000000..df302bac2
--- /dev/null
+++ b/installed-tests/js/testPromise.js
@@ -0,0 +1,106 @@
+// -*- mode: js; indent-tabs-mode: nil -*-
+// SPDX-License-Identifier: MIT OR LGPL-2.0-or-later
+// SPDX-FileContributor: Authored by: Marco Trevisan <marco trevisan canonical com>
+// SPDX-FileCopyrightText: 2022 Canonical, Ltd.
+
+const {GLib} = imports.gi;
+
+class SubPromise extends Promise {
+    constructor(executor) {
+        super((resolve, reject) => {
+            executor(resolve, reject);
+        });
+    }
+}
+
+const PromiseResult = {
+    RESOLVED: 0,
+    FAILED: 1,
+};
+
+describe('Promise', function () {
+    let loop;
+
+    beforeEach(function () {
+        loop = GLib.MainLoop.new(null, false);
+    });
+
+    function executePromise(promise) {
+        const promiseResult = {};
+
+        promise.then(value => {
+            promiseResult.result = PromiseResult.RESOLVED;
+            promiseResult.value = value;
+        }).catch(e => {
+            promiseResult.result = PromiseResult.FAILED;
+            promiseResult.error = e;
+        });
+
+        while (promiseResult.result === undefined)
+            loop.get_context().iteration(true);
+
+        return promiseResult;
+    }
+
+    it('waits for all promises before handling unhandled, when handled', function () {
+        let error;
+        let resolved;
+
+        const promise = async () => {
+            await new SubPromise(resolve => resolve('success'));
+            const rejecting = new SubPromise((_resolve, reject) => reject('got error'));
+            try {
+                await rejecting;
+            } catch (e) {
+                error = e;
+            } finally {
+                resolved = true;
+            }
+
+            expect(resolved).toBeTrue();
+            expect(error).toBe('got error');
+
+            return 'parent-returned';
+        };
+
+        expect(executePromise(promise())).toEqual({
+            result: PromiseResult.RESOLVED,
+            value: 'parent-returned',
+        });
+    });
+
+    it('waits for all promises before handling unhandled, when unhandled', function () {
+        const thenHandler = jasmine.createSpy('thenHandler');
+
+        const promise = async () => {
+            await new Promise(resolve => resolve('success'));
+            await new Promise((_resolve, reject) => reject(new Error('got error')));
+            return 'parent-returned';
+        };
+
+        promise().then(thenHandler).catch();
+        expect(thenHandler).not.toHaveBeenCalled();
+
+        GLib.idle_add(GLib.PRIORITY_DEFAULT_IDLE, () => loop.quit());
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_WARNING,
+            'Unhandled promise rejection.*');
+        loop.run();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testPromise.js', 0,
+            'warnsIfRejected');
+    });
+
+    it('do not lead to high-priority IDLE starvation', function () {
+        const promise = new Promise(resolve => {
+            const id = GLib.idle_add(GLib.PRIORITY_HIGH, () => {
+                resolve();
+                return GLib.SOURCE_REMOVE;
+            });
+            GLib.Source.set_name_by_id(id, `Test Idle source ${id}`);
+        });
+
+        expect(executePromise(promise)).toEqual({
+            result: PromiseResult.RESOLVED,
+            value: undefined,
+        });
+    });
+});