From 52adc35e38b2475fd6468982bb9ed31d50304f36 Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Sat, 4 Mar 2023 23:39:48 -0800 Subject: context: Fix job queue ending prematurely when dispatcher reset When the promise job dispatcher is reset, its GCancellable is replaced with a different instance. However, the original GCancellable instance was passed to GjsContextPrivate::runJobs(), which aborts the job queue if the GCancellable is cancelled. This wasn't normally a problem, because the GCancellable isn't usually cancelled. However, running JS with code coverage instrumentation starts and stops the job dispatcher after every job, in order for the debugger code to do its thing. So the GCancellable was getting cancelled and immediately replaced with a fresh one, but the job queue was still getting aborted. This still didn't seem to have any noticeable effect until we merged the runAsync() feature recently. From that point, async module evaluate operations started getting dropped on the floor when code coverage was active, which is very bad news. The fix is to not hold on to the GCancellable across jobs. Instead of checking whether the GCancellable is cancelled, instead check whether the promise job dispatcher is running. --- gjs/context-private.h | 5 +---- gjs/context.cpp | 10 ++++------ gjs/promise.cpp | 6 +----- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/gjs/context-private.h b/gjs/context-private.h index d8b7b85d..147feae4 100644 --- a/gjs/context-private.h +++ b/gjs/context-private.h @@ -17,7 +17,6 @@ #include // for pair #include -#include #include #include @@ -248,13 +247,11 @@ class GjsContextPrivate : public JS::JobQueue { JS::HandleObject allocation_site, JS::HandleObject incumbent_global) override; void runJobs(JSContext* cx) override; - void runJobs(JSContext* cx, GCancellable* cancellable); [[nodiscard]] bool empty() const override { return m_job_queue.empty(); } js::UniquePtr saveJobQueue( JSContext* cx) override; - GJS_JSAPI_RETURN_CONVENTION bool run_jobs_fallible( - GCancellable* cancellable = nullptr); + GJS_JSAPI_RETURN_CONVENTION bool run_jobs_fallible(); void register_unhandled_promise_rejection(uint64_t id, GjsAutoChar&& stack); void unregister_unhandled_promise_rejection(uint64_t id); void warn_about_unhandled_promise_rejections(); diff --git a/gjs/context.cpp b/gjs/context.cpp index 8d2cbc6e..59ac8afd 100644 --- a/gjs/context.cpp +++ b/gjs/context.cpp @@ -1045,12 +1045,10 @@ bool GjsContextPrivate::enqueuePromiseJob(JSContext* cx [[maybe_unused]], // Override of JobQueue::runJobs(). Called by js::RunJobs(), and when execution // of the job queue was interrupted by the debugger and is resuming. -void GjsContextPrivate::runJobs(JSContext* cx) { runJobs(cx, nullptr); } - -void GjsContextPrivate::runJobs(JSContext* cx, GCancellable* cancellable) { +void GjsContextPrivate::runJobs(JSContext* cx) { g_assert(cx == m_cx); g_assert(from_cx(cx) == this); - if (!run_jobs_fallible(cancellable)) + if (!run_jobs_fallible()) gjs_log_exception(cx); } @@ -1066,7 +1064,7 @@ void GjsContextPrivate::runJobs(JSContext* cx, GCancellable* cancellable) { * Returns: false if one of the jobs threw an uncatchable exception; * otherwise true. */ -bool GjsContextPrivate::run_jobs_fallible(GCancellable* cancellable) { +bool GjsContextPrivate::run_jobs_fallible() { bool retval = true; if (m_draining_job_queue || m_should_exit) @@ -1083,7 +1081,7 @@ bool GjsContextPrivate::run_jobs_fallible(GCancellable* cancellable) { * it's crucial to recheck the queue length during each iteration. */ for (size_t ix = 0; ix < m_job_queue.length(); ix++) { /* A previous job might have set this flag. e.g., System.exit(). */ - if (m_should_exit || g_cancellable_is_cancelled(cancellable)) { + if (m_should_exit || !m_dispatcher.is_running()) { gjs_debug(GJS_DEBUG_MAINLOOP, "Stopping jobs because of %s", m_should_exit ? "exit" : "main loop cancel"); break; diff --git a/gjs/promise.cpp b/gjs/promise.cpp index 0507eaf3..f248b019 100644 --- a/gjs/promise.cpp +++ b/gjs/promise.cpp @@ -82,12 +82,8 @@ class PromiseJobDispatcher::Source : public GSource { // next one to execute. (it will starve the other sources) g_source_set_ready_time(this, -1); - // A reference to the current cancellable is needed in case any - // jobs reset PromiseJobDispatcher and thus replace the cancellable. - GjsAutoUnref cancellable(m_cancellable, - GjsAutoTakeOwnership{}); // Drain the job queue. - m_gjs->runJobs(m_gjs->context(), cancellable); + m_gjs->runJobs(m_gjs->context()); return G_SOURCE_CONTINUE; } -- cgit v1.2.1