Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/jsc/bindings/webcore/AbortSignal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ JSValue AbortSignal::jsReason(JSC::JSGlobalObject& globalObject)
if (m_commonReason != CommonAbortReason::None) {
existingValue = toJS(&globalObject, m_commonReason);
m_commonReason = CommonAbortReason::None;
m_reason.setWeakly(existingValue);
m_reason.set(globalObject.vm(), wrapper(), existingValue);
}
}

Expand Down Expand Up @@ -159,10 +159,11 @@ void AbortSignal::markAborted(JSC::JSValue reason)
applyFlags(static_cast<uint8_t>(AbortSignalFlags::Aborted) | static_cast<uint8_t>(AbortSignalFlags::IsFiringEventListeners));
m_sourceSignals.clear();

// FIXME: This code is wrong: we should emit a write-barrier. Otherwise, GC can collect it.
// https://bugs.webkit.org/show_bug.cgi?id=236353
ASSERT(reason);
m_reason.setWeakly(reason);
if (auto* context = scriptExecutionContext())
m_reason.set(context->vm(), wrapper(), reason);
else
m_reason.setWeakly(reason);

cancelTimer();
}
Expand Down
22 changes: 21 additions & 1 deletion src/jsc/bindings/webcore/JSAbortController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,31 @@ void JSAbortController::visitChildrenImpl(JSCell* cell, Visitor& visitor)
ASSERT_GC_OBJECT_INHERITS(thisObject, info());
Base::visitChildren(thisObject, visitor);
addWebCoreOpaqueRoot(visitor, thisObject->wrapped().opaqueRoot());
thisObject->wrapped().signal().reason().visit(visitor);
thisObject->visitAdditionalChildrenInGCThread(visitor);
}

DEFINE_VISIT_CHILDREN(JSAbortController);

template<typename Visitor>
void JSAbortController::visitOutputConstraints(JSCell* cell, Visitor& visitor)
{
auto* thisObject = uncheckedDowncast<JSAbortController>(cell);
ASSERT_GC_OBJECT_INHERITS(thisObject, info());
Base::visitOutputConstraints(thisObject, visitor);
thisObject->visitAdditionalChildrenInGCThread(visitor);
}

template void JSAbortController::visitOutputConstraints(JSCell*, AbstractSlotVisitor&);
template void JSAbortController::visitOutputConstraints(JSCell*, SlotVisitor&);

template<typename Visitor>
void JSAbortController::visitAdditionalChildrenInGCThread(Visitor& visitor)
{
wrapped().signal().reason().visit(visitor);
}

DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD(JSAbortController);

void JSAbortController::analyzeHeap(JSCell* cell, HeapAnalyzer& analyzer)
{
auto* thisObject = uncheckedDowncast<JSAbortController>(cell);
Expand Down
2 changes: 2 additions & 0 deletions src/jsc/bindings/webcore/JSAbortController.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ class JSAbortController : public JSDOMWrapper<AbortController> {
}
static JSC::GCClient::IsoSubspace* subspaceForImpl(JSC::VM& vm);
DECLARE_VISIT_CHILDREN;
template<typename Visitor> void visitAdditionalChildrenInGCThread(Visitor&);

template<typename Visitor> static void visitOutputConstraints(JSCell*, Visitor&);
static void analyzeHeap(JSCell*, JSC::HeapAnalyzer&);

protected:
Expand Down
3 changes: 2 additions & 1 deletion src/jsc/bindings/webcore/JSValueInWrappedObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ inline void JSValueInWrappedObject::setWeakly(JSC::JSValue value)
inline void JSValueInWrappedObject::set(JSC::VM& vm, const JSC::JSCell* owner, JSC::JSValue value)
{
setWeakly(value);
vm.writeBarrier(owner, value);
if (owner)
vm.writeBarrier(owner, value);
}

inline void JSValueInWrappedObject::clear()
Expand Down
57 changes: 56 additions & 1 deletion test/js/web/abort/abort-controller-gc-reason.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,64 @@
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
import { bunEnv, bunExe, isWindows } from "harness";

// https://bugs.webkit.org/show_bug.cgi?id=293319
// AbortController.signal.reason is lost after garbage collection
describe("AbortController GC", () => {
// https://bugs.webkit.org/show_bug.cgi?id=236353
// verifyGC + collectContinuously is prohibitively slow under Windows CI and
// the code path under test is platform-independent.
test.skipIf(isWindows)(
"reason is marked across concurrent GC (write barrier / output constraint)",
async () => {
// verifyGC asserts on reachable cells the concurrent collector missed.
// Before the fix, abort() after the controller was scanned stored the
// reason with no write barrier / output constraint, leaving it unmarked.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const controllers = [];
for (let iter = 0; iter < 500; iter++) {
const c = new AbortController();
controllers.push(c);
const garbage = [];
for (let j = 0; j < 50; j++) garbage.push({ a: j, b: new Array(10).fill(j) });
c.abort(new Error("reason-" + iter));
}
let lost = 0;
for (const c of controllers) {
if (!(c.signal.reason instanceof Error)) lost++;
}
console.log("PASS lost=" + lost);
`,
],
env: {
...bunEnv,
BUN_JSC_verifyGC: "1",
BUN_JSC_collectContinuously: "1",
// `bun -e` sets numberOfGCMarkers=1 for one-shot startup, which
// serializes marking and hides the race. Restore parallel marking.
BUN_JSC_numberOfGCMarkers: "8",
},
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

// On failure verifyGC prints "GC Verifier: ERROR cell ... was not marked"
// to stderr and aborts before stdout is reached. stderr is included for
// diagnostics only; debug/ASAN builds may emit benign warnings there.
expect({ stdout: stdout.trim(), exitCode, stderr }).toEqual({
stdout: "PASS lost=0",
exitCode: 0,
stderr: expect.not.stringContaining("was not marked"),
});
},
120_000,
);

test("signal.reason survives GC when only controller is retained", async () => {
await using proc = Bun.spawn({
cmd: [
Expand Down
Loading