Skip to content

Commit 6a4d54c

Browse files
committed
Fix JsRpcPromise::Resolved context checking.
`IoContext::WeakRef` is a KJ I/O object, so `kj::Own<IoContext::WeakRef>` is not safe to hold from a JSG object. Instead, it would need to be `kj::IoOwn<IoContext::WeakRef>`. But, at that point we don't even need the `WeakRef` anymore because dereferencing the `IoOwn` itself will do exactly the check that we were using the `WeakRef` to do. Actually, all we really need here is an `IoPtr<T>` (the type of `T` is irrelevant) which we can attempt to dereference in order to effect the context check we want.
1 parent 93f4d6c commit 6a4d54c

3 files changed

Lines changed: 13 additions & 6 deletions

File tree

src/workerd/api/tests/js-rpc-test.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -883,7 +883,8 @@ export let crossContextSharingDoesntWork = {
883883
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
884884
"request/response bodies, and others) created in the context of one request handler " +
885885
"cannot be accessed from a different request's handler. This is a limitation of " +
886-
"Cloudflare Workers which allows us to improve overall performance."
886+
"Cloudflare Workers which allows us to improve overall performance. " +
887+
"(I/O type: JsRpcPromise)"
887888
});
888889

889890
// Now let's try accessing a JsRpcProperty, where the property is NOT a direct property of a
@@ -907,7 +908,8 @@ export let crossContextSharingDoesntWork = {
907908
"Cannot perform I/O on behalf of a different request. I/O objects (such as streams, " +
908909
"request/response bodies, and others) created in the context of one request handler " +
909910
"cannot be accessed from a different request's handler. This is a limitation of " +
910-
"Cloudflare Workers which allows us to improve overall performance."
911+
"Cloudflare Workers which allows us to improve overall performance. " +
912+
"(I/O type: JsRpcPromise)"
911913
});
912914
},
913915
}

src/workerd/api/worker-rpc.c++

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ void JsRpcPromise::resolve(jsg::Lock& js, jsg::JsValue result) {
339339
if (state.is<Pending>()) {
340340
state = Resolved {
341341
.result = jsg::Value(js.v8Isolate, result),
342-
.ioCtx = IoContext::current().getWeakRef()
342+
.ctxCheck = IoContext::current().addObject(*this),
343343
};
344344
} else {
345345
// We'd better dispose this.
@@ -370,7 +370,9 @@ rpc::JsRpcTarget::Client JsRpcPromise::getClientForOneCall(
370370
return pending.pipeline->getCallPipeline();
371371
}
372372
KJ_CASE_ONEOF(resolved, Resolved) {
373-
IoContext::requireCurrentOrThrowJs(*resolved.ioCtx);
373+
// Dereference `ctxCheck` just to verify we're running in the correct context. (If not,
374+
// this will throw.)
375+
*resolved.ctxCheck;
374376

375377
// A value was already returned, and we closed the original RPC pipeline. But the application
376378
// kept the promise around and is still trying to pipeline on it. What do we do?

src/workerd/api/worker-rpc.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,11 @@ class JsRpcPromise: public JsRpcClientProvider {
213213
struct Resolved {
214214
jsg::Value result;
215215

216-
// We only use this to prohibit use from the wrong context.
217-
kj::Own<IoContext::WeakRef> ioCtx;
216+
// Dummy IoPtr to self, used only to verify that we're running in the correct context.
217+
// (Dereferencing from the wrong context would throw an exception.)
218+
// Note: Can't use IoContext::WeakRef here because it's not thread-safe (it's only intended to
219+
// be helf from KJ I/O objects, but this is a JSG object).
220+
IoPtr<JsRpcPromise> ctxCheck;
218221
};
219222
struct Disposed {};
220223

0 commit comments

Comments
 (0)