Add support for retrieving the Engine from a JSContext

... and use that wherever we previously stored a pointer to the `Engine` in a static global.

Signed-off-by: Till Schneidereit <till@tillschneidereit.net>

Author: tschneidereit

builtins/web/fetch/fetch-api.cpp

@@ -9,8 +9,6 @@
 
 namespace builtins::web::fetch {
 
-static api::Engine *ENGINE;
-
 // TODO: throw in all Request methods/getters that rely on host calls once a
 // request has been sent. The host won't let us act on them anymore anyway.
 /**
@@ -86,7 +84,8 @@ bool fetch(JSContext *cx, unsigned argc, Value *vp) {
   // If the request body is streamed, we need to wait for streaming to complete
   // before marking the request as pending.
   if (!streaming) {
-    ENGINE->queue_async_task(new ResponseFutureTask(request_obj, pending_handle));
+    api::Engine::from_context(cx)
+      .queue_async_task(new ResponseFutureTask(request_obj, pending_handle));
   }
 
   SetReservedSlot(request_obj, static_cast<uint32_t>(Request::Slots::ResponsePromise),
@@ -101,8 +100,6 @@ bool fetch(JSContext *cx, unsigned argc, Value *vp) {
 const JSFunctionSpec methods[] = {JS_FN("fetch", fetch, 2, JSPROP_ENUMERATE), JS_FS_END};
 
 bool install(api::Engine *engine) {
-  ENGINE = engine;
-
   if (!JS_DefineFunctions(engine->cx(), engine->global(), methods))
     return false;
   if (!request_response::install(engine)) {

builtins/web/fetch/request-response.cpp

@@ -41,8 +41,6 @@ bool NativeStreamSource::stream_is_body(JSContext *cx, JS::HandleObject stream)
 
 namespace builtins::web::fetch {
 
-static api::Engine *ENGINE;
-
 bool error_stream_controller_with_pending_exception(JSContext *cx, HandleObject controller) {
   RootedValue exn(cx);
   if (!JS_GetPendingException(cx, &exn))
@@ -466,7 +464,8 @@ bool finish_outgoing_body_streaming(JSContext* cx, HandleObject body_owner) {
             .toPrivate());
     SetReservedSlot(body_owner, static_cast<uint32_t>(Request::Slots::PendingResponseHandle),
                     PrivateValue(nullptr));
-    ENGINE->queue_async_task(new ResponseFutureTask(body_owner, pending_handle));
+    api::Engine::from_context(cx)
+      .queue_async_task(new ResponseFutureTask(body_owner, pending_handle));
   }
 
   return true;
@@ -476,9 +475,10 @@ bool RequestOrResponse::append_body(JSContext *cx, JS::HandleObject self, JS::Ha
   MOZ_ASSERT(!body_used(source));
   MOZ_ASSERT(!body_used(self));
   MOZ_ASSERT(self != source);
+  auto engine = api::Engine::from_context(cx);
   host_api::HttpIncomingBody *source_body = incoming_body_handle(source);
   host_api::HttpOutgoingBody *dest_body = outgoing_body_handle(self);
-  auto res = dest_body->append(ENGINE, source_body, finish_outgoing_body_streaming, self);
+  auto res = dest_body->append(&engine, source_body, finish_outgoing_body_streaming, self);
   if (auto *err = res.to_err()) {
     HANDLE_ERROR(cx, *err);
     return false;
@@ -892,7 +892,7 @@ bool RequestOrResponse::body_source_pull_algorithm(JSContext *cx, CallArgs args,
     }
   }
 
-  ENGINE->queue_async_task(new BodyFutureTask(source));
+  api::Engine::from_context(cx).queue_async_task(new BodyFutureTask(source));
 
   args.rval().setUndefined();
   return true;
@@ -945,7 +945,7 @@ bool reader_for_outgoing_body_then_handler(JSContext *cx, JS::HandleObject body_
     // Uint8Array?
     fprintf(stderr, "Error: read operation on body ReadableStream didn't respond with a "
                     "Uint8Array. Received value: ");
-    ENGINE->dump_value(val, stderr);
+    api::Engine::from_context(cx).dump_value(val, stderr);
     return false;
   }
 
@@ -985,7 +985,7 @@ bool reader_for_outgoing_body_catch_handler(JSContext *cx, JS::HandleObject body
   // stream errored during the streaming send. Not much we can do, but at least
   // close the stream, and warn.
   fprintf(stderr, "Warning: body ReadableStream closed during body streaming. Exception: ");
-  ENGINE->dump_value(args.get(0), stderr);
+  api::Engine::from_context(cx).dump_value(args.get(0), stderr);
 
   return finish_outgoing_body_streaming(cx, body_owner);
 }
@@ -1004,7 +1004,8 @@ bool RequestOrResponse::maybe_stream_body(JSContext *cx, JS::HandleObject body_o
   if (is_incoming(body_owner)) {
     auto *source_body = incoming_body_handle(body_owner);
     auto *dest_body = destination->body().unwrap();
-    auto res = dest_body->append(ENGINE, source_body, finish_outgoing_body_streaming, nullptr);
+    auto engine = api::Engine::from_context(cx);
+    auto res = dest_body->append(&engine, source_body, finish_outgoing_body_streaming, nullptr);
     if (auto *err = res.to_err()) {
       HANDLE_ERROR(cx, *err);
       return false;
@@ -1233,7 +1234,7 @@ bool Request::clone(JSContext *cx, unsigned argc, JS::Value *vp) {
   Value url_val = GetReservedSlot(self, static_cast<uint32_t>(Slots::URL));
   SetReservedSlot(new_request, static_cast<uint32_t>(Slots::URL), url_val);
   Value method_val = JS::StringValue(method(self));
-  ENGINE->dump_value(method_val, stderr);
+  api::Engine::from_context(cx).dump_value(method_val, stderr);
   SetReservedSlot(new_request, static_cast<uint32_t>(Slots::Method), method_val);
 
   // clone operation step 2.
@@ -2459,8 +2460,6 @@ JSObject * Response::create_incoming(JSContext *cx, host_api::HttpIncomingRespon
 namespace request_response {
 
 bool install(api::Engine *engine) {
-  ENGINE = engine;
-
   if (!Request::init_class(engine->cx(), engine->global()))
     return false;
   if (!Response::init_class(engine->cx(), engine->global()))

builtins/web/timers.cpp

@@ -28,7 +28,6 @@ class TimersMap {
 }
 
 static PersistentRooted<js::UniquePtr<TimersMap>> TIMERS_MAP;
-static api::Engine *ENGINE;
 
 class TimerTask final : public api::AsyncTask {
   using TimerArgumentsVector = std::vector<JS::Heap<JS::Value>>;
@@ -119,12 +118,12 @@ class TimerTask final : public api::AsyncTask {
     }
   }
 
-  static bool clear(int32_t timer_id) {
+  static bool clear(api::Engine &engine, int32_t timer_id) {
     if (!TIMERS_MAP->timers_.contains(timer_id)) {
       return false;
     }
 
-    ENGINE->cancel_async_task(TIMERS_MAP->timers_[timer_id]);
+    engine.cancel_async_task(TIMERS_MAP->timers_[timer_id]);
     TIMERS_MAP->timers_.erase(timer_id);
     return true;
   }
@@ -171,7 +170,7 @@ template <bool repeat> bool setTimeout_or_interval(JSContext *cx, const unsigned
   }
 
   const auto timer = new TimerTask(delay, repeat, handler, handler_args);
-  ENGINE->queue_async_task(timer);
+  api::Engine::from_context(cx).queue_async_task(timer);
   args.rval().setInt32(timer->timer_id());
 
   return true;
@@ -193,7 +192,7 @@ template <bool interval> bool clearTimeout_or_interval(JSContext *cx, unsigned a
     return false;
   }
 
-  TimerTask::clear(id);
+  TimerTask::clear(api::Engine::from_context(cx), id);
 
   args.rval().setUndefined();
   return true;
@@ -206,7 +205,6 @@ constexpr JSFunctionSpec methods[] = {
     JS_FN("clearTimeout", clearTimeout_or_interval<false>, 1, JSPROP_ENUMERATE), JS_FS_END};
 
 bool install(api::Engine *engine) {
-  ENGINE = engine;
   TIMERS_MAP.init(engine->cx(), js::MakeUnique<TimersMap>());
   return JS_DefineFunctions(engine->cx(), engine->global(), methods);
 }

include/extension-api.h

@@ -36,7 +36,27 @@ class Engine {
 
 public:
   Engine();
-  JSContext *cx();
+
+  /**
+   * Returns the `JSContext` associated with the `Engine` instance.
+   *
+   * Currently, StarlingMonkey uses exactly one `Engine` and one `JSContext`. Since that might
+   * change in the future, it's heavily recommended to not store either in static globals.
+   *
+   * @return The JSContext associated with the `Engine` instance
+   */
+ JSContext *cx();
+
+ /**
+  * Returns the `Engine` associated with the `JSContext` instance.
+  *
+  * Currently, StarlingMonkey uses exactly one `Engine` and one `JSContext`. Since that might
+  * change in the future, it's heavily recommended to not store either in static globals.
+  *
+  * @return The `Engine` associated with the `JSContext` instance
+  */
+  static Engine& from_context(JSContext *cx);
+
   JS::HandleObject global();
 
   /// Initialize the engine with the given filename

runtime/cpp/engine.cpp

@@ -341,6 +341,12 @@ api::Engine::Engine() {
 
 JSContext *api::Engine::cx() { return CONTEXT; }
 
+api::Engine &api::Engine::from_context(JSContext *cx) {
+  // This could also be implemented by storing the engine on the cx using JS_SetContextPrivate.
+  // Since we're not supporting multiple contexts or engines, we can just return the global engine.
+  return *ENGINE;
+}
+
 HandleObject api::Engine::global() { return GLOBAL; }
 
 extern bool install_builtins(api::Engine *engine);