Worker: swap process.env for SHARE_ENV only after the worker thread is spawned

src/js/internal-for-testing.ts

@@ -666,6 +666,8 @@ export const structuredCloneAdvanced: (
   forTransfer: boolean,
   forStorage: boolean,
   serializationContext: SerializationContext,
+  /** Runs once the value is serialized, before a transfer list that is not empty is detached. */
+  beforeTransfer?: () => void,
 ) => any = $newCppFunction("StructuredClone.cpp", "jsFunctionStructuredCloneAdvanced", 5);
 
 export const isASANEnabled: () => boolean = $newCppFunction("InternalForTesting.cpp", "jsFunction_isASANEnabled", 0);

src/jsc/bindings/JSEnvironmentVariableMap.cpp

@@ -904,7 +904,7 @@ bool isProcessEnvClassInfo(const JSC::ClassInfo* classInfo)
     return classInfo == JSEnvironmentVariableMap::info() || classInfo == JSSharedEnvMap::info();
 }
 
-RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalObject)
+RefPtr<SharedEnvStore> sharedEnvStoreForNewWorker(Zig::GlobalObject* globalObject)
 {
     VM& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -914,8 +914,7 @@ RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalOb
     if (auto* existing = sharedEnvStoreFor(globalObject))
         return existing;
 
-    // Founding a new tree. processEnvObject() forces the lazy init so the OS
-    // environment is captured before the swap below.
+    // Seeding a new tree. processEnvObject() forces the lazy init so the OS environment is captured.
     JSObject* envObject = globalObject->processEnvObject();
     if (!envObject->staticPropertiesReified()) {
         envObject->reifyAllStaticProperties(globalObject);
@@ -941,12 +940,35 @@ RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalOb
 
     // Enumerating or reading process.env can run user JS (an accessor, or Windows'
     // Proxy traps) that spawns a SHARE_ENV worker and founds the tree first. Defer
-    // to it instead of overwriting its store and re-swapping process.env.
+    // to it instead of handing the new worker a store this thread will never join.
     if (auto* existing = sharedEnvStoreFor(globalObject))
         return existing;
 
+    return store;
+}
+
+JSValue processEnvProperty(Zig::GlobalObject* globalObject)
+{
+    if (!globalObject->hasProcessObject())
+        return {};
+    VM& vm = globalObject->vm();
+    return globalObject->processObject()->getDirect(vm, JSC::Identifier::fromString(vm, "env"_s));
+}
+
+void joinSharedEnvTree(Zig::GlobalObject* globalObject, SharedEnvStore& store, JSValue processEnvPropertyBefore)
+{
+    if (auto* existing = sharedEnvStoreFor(globalObject)) {
+        ASSERT_UNUSED(existing, existing == &store);
+        return;
+    }
+
+    VM& vm = globalObject->vm();
+    // sharedEnvStoreForNewWorker() read it to seed the store, so this runs no lazy initializer.
+    ASSERT(globalObject->m_processEnvObject.isInitialized());
+    JSObject* previous = globalObject->processEnvObject();
+
     // Publish before creating the view, which resolves its store via the context.
-    globalObject->scriptExecutionContext()->setSharedEnvStore(store.get());
+    globalObject->scriptExecutionContext()->setSharedEnvStore(store);
 
     // Swap this global's process.env to the shared, write-through variant.
     auto* shared = createSharedEnvironmentVariablesMap(globalObject).getObject();
@@ -958,7 +980,10 @@ RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalOb
     // overwrite it so it resolves to the shared variant.
     if (globalObject->hasProcessObject()) {
         JSObject* processObject = globalObject->processObject();
-        processObject->putDirect(vm, envIdentifier, shared, 0);
+        JSValue current = processObject->getDirect(vm, envIdentifier);
+        // What script assigned or deleted during the constructor stays, as it did when the swap ran before serialization.
+        if (current && (current == previous || current == processEnvPropertyBefore))
+            processObject->putDirect(vm, envIdentifier, shared, 0);
     }
 
     // Bun.env reifies to the same object at startup; repoint it too, or it keeps
@@ -968,8 +993,6 @@ RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalOb
         if (bunObject->getDirect(vm, envIdentifier))
             bunObject->putDirect(vm, envIdentifier, shared, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete);
     }
-
-    return store;
 }
 
 JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject)

src/jsc/bindings/JSEnvironmentVariableMap.h

@@ -74,8 +74,14 @@ JSC::JSValue createSharedEnvironmentVariablesMap(Zig::GlobalObject* globalObject
 bool isProcessEnvClassInfo(const JSC::ClassInfo*);
 
 // SHARE_ENV store for a worker spawned from `globalObject`: the spawner's existing store,
-// else a fresh one seeded from its `process.env` (then swapped to a write-through view).
+// else a fresh one seeded from its `process.env`, which no thread has joined yet.
 // Returns null if seeding threw.
-RefPtr<SharedEnvStore> ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalObject);
+RefPtr<SharedEnvStore> sharedEnvStoreForNewWorker(Zig::GlobalObject* globalObject);
+
+// The `env` property of `process` as it is now, for joinSharedEnvTree(). Empty if the property is not reified.
+JSC::JSValue processEnvProperty(Zig::GlobalObject* globalObject);
+
+// Publishes the store and swaps `process.env` to a write-through view of it, unless the thread is in a tree already. Runs no JS and cannot throw.
+void joinSharedEnvTree(Zig::GlobalObject* globalObject, SharedEnvStore&, JSC::JSValue processEnvPropertyBefore);
 
 }

src/jsc/bindings/webcore/JSWorker.cpp

@@ -71,6 +71,7 @@
 #include <wtf/GetPtr.h>
 #include <wtf/PointerPreparations.h>
 #include <wtf/Scope.h>
+#include <wtf/ScopedLambda.h>
 #include <wtf/URL.h>
 #include "SerializedScriptValue.h"
 #include "BunProcess.h"
@@ -165,8 +166,7 @@ template<> __attribute__((minsize)) JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES
     EnsureStillAliveScope argument1 = callFrame->argument(1);
 
     WorkerOptions options {};
-    // Founding an env tree swaps the parent's process.env, so it is deferred until
-    // every option has validated (below).
+    // Joining an env tree swaps the parent's process.env for good, so it waits until the worker has a thread (joinSharedEnvTree below).
     bool shareEnv = false;
     JSValue nodeWorkerObject {};
     if (callFrame->argumentCount() == 3) {
@@ -358,14 +358,13 @@ template<> __attribute__((minsize)) JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES
     setSubclassStructureIfNeeded<Worker>(lexicalGlobalObject, callFrame, asObject(jsValue));
     RETURN_IF_EXCEPTION(throwScope, {});
 
-    // Resolve the spawning thread's env tree (founding one if needed) so disjoint
-    // SHARE_ENV chains stay isolated. Runs only after Worker::create() validated the options,
-    // because founding a tree swaps this thread's process.env.
+    // The spawning thread's env store, or an unpublished seed. Read after serialization (a getter's write is in it) and before the transfer (the read can throw).
     RefPtr<Bun::SharedEnvStore> sharedEnvStore;
-    if (shareEnv) {
-        sharedEnvStore = Bun::ensureSharedEnvStoreForWorker(globalObject);
-        RETURN_IF_EXCEPTION(throwScope, {});
-    }
+    auto resolveSharedEnvStore = [&] {
+        sharedEnvStore = Bun::sharedEnvStoreForNewWorker(globalObject);
+    };
+    const ScopedLambda<void()> beforeTransfer = resolveSharedEnvStore;
+    EnsureStillAliveScope processEnvPropertyBefore = shareEnv ? Bun::processEnvProperty(globalObject) : JSValue();
 
     // A worker made by script of a Bun.ModuleGraph that was already disposed is terminated at birth
     // and reports nothing (Worker::create), so nothing is sent to it. The ports node:worker_threads
@@ -389,13 +388,18 @@ template<> __attribute__((minsize)) JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES
 
     // Reports a DataCloneError through ExceptionOr, but a getter or toJSON that throws during serialization
     // leaves that exception on the VM and returns normally.
-    ExceptionOr<Ref<SerializedScriptValue>> serialized = SerializedScriptValue::create(*lexicalGlobalObject, valueToTransfer, WTF::move(transferList), ports, SerializationForStorage::No, SerializationContext::WorkerPostMessage);
+    ExceptionOr<Ref<SerializedScriptValue>> serialized = SerializedScriptValue::create(*lexicalGlobalObject, valueToTransfer, WTF::move(transferList), ports, SerializationForStorage::No, SerializationContext::WorkerPostMessage, SerializationForCrossProcessTransfer::No, shareEnv ? &beforeTransfer : nullptr);
     RETURN_IF_EXCEPTION(throwScope, {});
     if (serialized.hasException()) {
         WebCore::propagateException(*lexicalGlobalObject, throwScope, serialized.releaseException());
         RELEASE_AND_RETURN(throwScope, {});
     }
     RETURN_IF_EXCEPTION(throwScope, {});
+    // create() runs beforeTransfer only for a non-empty transfer list. A worker stopped at birth gets no thread, so no store.
+    if (shareEnv && !sharedEnvStore && !worker->hasExited()) {
+        resolveSharedEnvStore();
+        RETURN_IF_EXCEPTION(throwScope, {});
+    }
 
     Vector<TransferredMessagePort> transferredPorts;
 
@@ -408,12 +412,17 @@ template<> __attribute__((minsize)) JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES
         transferredPorts = disentangleResult.releaseReturnValue();
     }
 
-    auto started = worker->start(serialized.releaseReturnValue(), WTF::move(transferredPorts), WTF::move(sharedEnvStore));
+    auto started = worker->start(serialized.releaseReturnValue(), WTF::move(transferredPorts), sharedEnvStore.copyRef());
     discardUnstarted.release();
     if (started.hasException()) [[unlikely]] {
         WebCore::propagateException(*lexicalGlobalObject, throwScope, started.releaseException());
         RELEASE_AND_RETURN(throwScope, {});
     }
+    // start() spawns no thread for a worker that was stopped first: hasExited() is true for that one.
+    if (sharedEnvStore && !worker->hasExited()) {
+        Bun::joinSharedEnvTree(globalObject, *sharedEnvStore, processEnvPropertyBefore.value());
+        throwScope.assertNoExceptionExceptTermination();
+    }
 
     // Emit the 'worker' event on the process. If we are constructing a `node:worker_threads`
     // worker, we emit the event with the worker_threads Worker object instead of the Web Worker.

src/jsc/bindings/webcore/SerializedScriptValue.cpp

@@ -85,6 +85,7 @@
 #include <wtf/CompletionHandler.h>
 #include <wtf/MainThread.h>
 #include <wtf/RunLoop.h>
+#include <wtf/ScopedLambda.h>
 #include <wtf/Vector.h>
 #include <wtf/threads/BinarySemaphore.h>
 
@@ -4481,18 +4482,18 @@ static bool isObjectFastPathCandidate(Structure* structure)
 RefPtr<SerializedScriptValue> SerializedScriptValue::create(JSC::JSGlobalObject& globalObject, JSC::JSValue value, SerializationForStorage forStorage, SerializationErrorMode throwExceptions, SerializationContext serializationContext, SerializationForCrossProcessTransfer forTransfer)
 {
     Vector<RefPtr<MessagePort>> dummyPorts;
-    auto result = create(globalObject, value, {}, dummyPorts, forStorage, throwExceptions, serializationContext, forTransfer);
+    auto result = create(globalObject, value, {}, dummyPorts, forStorage, throwExceptions, serializationContext, forTransfer, nullptr);
     if (result.hasException())
         return nullptr;
     return result.releaseReturnValue();
 }
 
-ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalObject& globalObject, JSValue value, Vector<JSC::Strong<JSC::JSObject>>&& transferList, Vector<RefPtr<MessagePort>>& messagePorts, SerializationForStorage forStorage, SerializationContext serializationContext, SerializationForCrossProcessTransfer forTransfer)
+ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalObject& globalObject, JSValue value, Vector<JSC::Strong<JSC::JSObject>>&& transferList, Vector<RefPtr<MessagePort>>& messagePorts, SerializationForStorage forStorage, SerializationContext serializationContext, SerializationForCrossProcessTransfer forTransfer, const ScopedLambda<void()>* beforeTransfer)
 {
-    return create(globalObject, value, WTF::move(transferList), messagePorts, forStorage, SerializationErrorMode::Throwing, serializationContext, forTransfer);
+    return create(globalObject, value, WTF::move(transferList), messagePorts, forStorage, SerializationErrorMode::Throwing, serializationContext, forTransfer, beforeTransfer);
 }
 
-ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalObject& lexicalGlobalObject, JSValue value, Vector<JSC::Strong<JSC::JSObject>>&& transferList, Vector<RefPtr<MessagePort>>& messagePorts, SerializationForStorage forStorage, SerializationErrorMode throwExceptions, SerializationContext context, SerializationForCrossProcessTransfer forTransfer)
+ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalObject& lexicalGlobalObject, JSValue value, Vector<JSC::Strong<JSC::JSObject>>&& transferList, Vector<RefPtr<MessagePort>>& messagePorts, SerializationForStorage forStorage, SerializationErrorMode throwExceptions, SerializationContext context, SerializationForCrossProcessTransfer forTransfer, const ScopedLambda<void()>* beforeTransfer)
 {
     VM& vm = lexicalGlobalObject.vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -4805,6 +4806,14 @@ ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalOb
     // Most calls have no transfer list; this keeps them at one branch.
     std::unique_ptr<ArrayBufferContentsArray> arrayBufferContentsArray;
     if (!transferList.isEmpty()) {
+        // Can run user code, as serializing did: the check below covers both.
+        if (beforeTransfer) [[unlikely]] {
+            (*beforeTransfer)();
+            if (scope.exception()) [[unlikely]] {
+                releaseSerializedBlockListRefs();
+                RELEASE_AND_RETURN(scope, Exception { ExistingExceptionError });
+            }
+        }
         if (auto exception = transferListChangedDuringSerialization(vm, transferList, arrayBuffers, messagePorts)) [[unlikely]] {
             releaseSerializedBlockListRefs();
             RELEASE_AND_RETURN(scope, WTF::move(*exception));

src/jsc/bindings/webcore/SerializedScriptValue.h

@@ -112,7 +112,8 @@ class SerializedScriptValue : public ThreadSafeRefCounted<SerializedScriptValue>
     static SYSV_ABI void writeBytesForBun(CloneSerializer*, const uint8_t*, uint32_t);
     static SYSV_ABI bool isTransferable(JSC::JSGlobalObject* globalObject, JSC::JSValue value);
 
-    WEBCORE_EXPORT static ExceptionOr<Ref<SerializedScriptValue>> create(JSC::JSGlobalObject&, JSC::JSValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer, Vector<RefPtr<MessagePort>>&, SerializationForStorage = SerializationForStorage::No, SerializationContext = SerializationContext::Default, SerializationForCrossProcessTransfer = SerializationForCrossProcessTransfer::No);
+    // `beforeTransfer` runs after the value is serialized and before a non-empty `transfer` is detached. It may throw and may run user code.
+    WEBCORE_EXPORT static ExceptionOr<Ref<SerializedScriptValue>> create(JSC::JSGlobalObject&, JSC::JSValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer, Vector<RefPtr<MessagePort>>&, SerializationForStorage = SerializationForStorage::No, SerializationContext = SerializationContext::Default, SerializationForCrossProcessTransfer = SerializationForCrossProcessTransfer::No, const ScopedLambda<void()>* beforeTransfer = nullptr);
 
     WEBCORE_EXPORT static RefPtr<SerializedScriptValue> create(JSC::JSGlobalObject&, JSC::JSValue, SerializationForStorage = SerializationForStorage::No, SerializationErrorMode = SerializationErrorMode::Throwing, SerializationContext = SerializationContext::Default, SerializationForCrossProcessTransfer = SerializationForCrossProcessTransfer::No);
 
@@ -153,7 +154,7 @@ class SerializedScriptValue : public ThreadSafeRefCounted<SerializedScriptValue>
     WEBCORE_EXPORT ~SerializedScriptValue();
 
 private:
-    static ExceptionOr<Ref<SerializedScriptValue>> create(JSC::JSGlobalObject&, JSC::JSValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer, Vector<RefPtr<MessagePort>>&, SerializationForStorage, SerializationErrorMode, SerializationContext, SerializationForCrossProcessTransfer);
+    static ExceptionOr<Ref<SerializedScriptValue>> create(JSC::JSGlobalObject&, JSC::JSValue, Vector<JSC::Strong<JSC::JSObject>>&& transfer, Vector<RefPtr<MessagePort>>&, SerializationForStorage, SerializationErrorMode, SerializationContext, SerializationForCrossProcessTransfer, const ScopedLambda<void()>* beforeTransfer);
     WEBCORE_EXPORT SerializedScriptValue(Vector<unsigned char>&&, std::unique_ptr<ArrayBufferContentsArray>&& = nullptr);
 
     SerializedScriptValue(Vector<unsigned char>&&, std::unique_ptr<ArrayBufferContentsArray>, std::unique_ptr<ArrayBufferContentsArray> sharedBuffers

src/jsc/bindings/webcore/StructuredClone.cpp

@@ -33,6 +33,7 @@
 #include "SerializedScriptValue.h"
 #include "MessagePort.h"
 #include "JSStructuredSerializeOptions.h"
+#include <wtf/ScopedLambda.h>
 
 namespace WebCore {
 using namespace JSC;
@@ -90,6 +91,7 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionStructuredCloneAdvanced, (JSC::JSGlobalObject
     bool isForTransfer = callFrame->argument(2).toBoolean(globalObject);
     bool isForStorage = callFrame->argument(3).toBoolean(globalObject);
     JSC::JSValue serializationContextValue = callFrame->argument(4);
+    JSC::JSValue beforeTransferValue = callFrame->argument(5);
 
     SerializationContext serializationContext = SerializationContext::Default;
     if (serializationContextValue.isString()) {
@@ -127,8 +129,13 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionStructuredCloneAdvanced, (JSC::JSGlobalObject
         }
     }
 
+    auto callBeforeTransfer = [&] {
+        JSC::call(globalObject, beforeTransferValue, JSC::ArgList(), "beforeTransfer is not a function"_s);
+    };
+    const ScopedLambda<void()> beforeTransfer = callBeforeTransfer;
+
     Vector<RefPtr<MessagePort>> ports;
-    ExceptionOr<Ref<SerializedScriptValue>> serialized = SerializedScriptValue::create(*globalObject, value, WTF::move(transferList), ports, forStorage, serializationContext, forTransfer);
+    ExceptionOr<Ref<SerializedScriptValue>> serialized = SerializedScriptValue::create(*globalObject, value, WTF::move(transferList), ports, forStorage, serializationContext, forTransfer, beforeTransferValue.isUndefined() ? nullptr : &beforeTransfer);
     RETURN_IF_EXCEPTION(throwScope, {});
     if (serialized.hasException()) {
         WebCore::propagateException(*globalObject, throwScope, serialized.releaseException());