From fbe58e9770bf7c130f7de46cd926dafcd9118f48 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 17 Sep 2026 09:07:10 -0700 Subject: [PATCH] fix: do not read `this` before `super()` in wrapped constructors A derived constructor leaves `this` unbound until `super()` returns. The wrapper read `this` in two places that can theoretically run before that point. Bun rejected both. Node rejected one, and accepted the other on every path except the error path. On both engines, however, the transform was incorrect. This showed up loudly in Sentry's instrumentation of Hono on Bun. See: https://github.com/getsentry/sentry-javascript/pull/24371 In JavaScriptCore an arrow function keeps the value of `this` that it read when it was entered. The wrapper moves the original body into a nested arrow, so `super()` now runs while the wrapper's own `runStores` callback is on the stack. That callback keeps the unbound value, and the `finally` block throws when it reads `this`: ``` ReferenceError: 'super()' must be called in derived constructor before accessing |this| or returning non-object. ``` V8 re-reads `this` from the environment, so Node did not show this. `captureSelfAtSuper` now rewrites each `super(...)` call in a constructor body to also record `this`, and the wrapper reads that variable to fill in `message.self`: ```js let tr_ch_apm$Undici_constructor$self; ... (super(val), tr_ch_apm$Undici_constructor$self = this); ... __apm$ctx.self ??= tr_ch_apm$Undici_constructor$self; ``` `super(...)` already evaluates to the newly bound `this`, so the sequence expression returns the same value the call did. The read now sits immediately after `super()`, which makes the order static. The variable name carries the channel, so a second channel that wraps the same constructor writes its own binding instead of shadowing the first. A `super()` call inside a nested class or a nested non-arrow function belongs to that function, so it keeps its original form. This also stops the wrapper from hiding an error, which is a pre-existing bug in Node.js related to this. A throw before `super()` used to surface a `ReferenceError` in place of the original error. The recorded variable is `undefined` when `super()` never ran, so the original error now propagates. The correct fix for this is to statically ensure that there is no way to access `this` before calling `super()`. `traceFunction` no longer calls `wrapSuper` when the body lands in an arrow function. An arrow inherits `super` from its enclosing method, so such a body keeps working without a rewrite. The rewrite was wrong for a constructor: it hoists a `super['x']` capture to the top of the constructor, above the body's own `super()` call, and that capture uses `this` as the receiver. As a result `super.method()` in a derived constructor failed on Node and on Bun. Methods and generators still get the rewrite, because a real `function` wrapper does lose the binding. The tests now run every fixture under Bun as well, when Bun is installed. Bun is not an officially supported engine, but people do run this code under it, and its stricter parsing can help surface latent bugs. Two fixtures are added. `constructor_self_cjs` covers `message.self` and the throw before `super()`. The fixture `constructor_super_method_cjs` covers `super.method()` in a derived constructor. Co-Authored-By: Claude Opus 5 (1M context) --- lib/transforms.js | 102 +++++++++++++++++++-- tests/constructor_self_cjs/mod.js | 19 ++++ tests/constructor_self_cjs/test.js | 30 ++++++ tests/constructor_super_method_cjs/mod.js | 22 +++++ tests/constructor_super_method_cjs/test.js | 19 ++++ tests/tests.test.mjs | 46 ++++++++-- 6 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 tests/constructor_self_cjs/mod.js create mode 100644 tests/constructor_self_cjs/test.js create mode 100644 tests/constructor_super_method_cjs/mod.js create mode 100644 tests/constructor_super_method_cjs/test.js diff --git a/lib/transforms.js b/lib/transforms.js index 7716c1d..967fd84 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -250,6 +250,15 @@ function traceFunction (state, node, program) { const type = isConstructor ? 'ArrowFunctionExpression' : node.type const params = node.params + // A derived constructor cannot read `this` in the wrapper, so record it at + // the `super()` call sites and point the wrapper at that binding instead. + // The name is namespaced by channel so that a second channel wrapping the + // same constructor writes to its own binding rather than shadowing ours. + const selfBinding = `${formatChannelVariable(state.channelName)}$self` + state.selfBinding = isConstructor && captureSelfAtSuper(node, selfBinding) + ? selfBinding + : null + node.body = wrap(state, { type, params, @@ -268,7 +277,13 @@ function traceFunction (state, node, program) { node.generator = false node.async = false - wrapSuper(state, node) + // An arrow function inherits `super` from its enclosing method, so a body + // moved into one keeps working and needs no rewrite. Rewriting it there is + // in fact wrong for a constructor: the capture it hoists reads `super.x` + // with `this` as the receiver, above the body's own `super()` call, where + // `this` is still unbound. Only the real `function` wrapper, which + // generators need, loses the binding. + if (type !== 'ArrowFunctionExpression') wrapSuper(state, node) } /** @@ -342,6 +357,7 @@ function traceInstanceMethod (state, node, program) { fn.params = [{ type: 'RestElement', argument: { type: 'Identifier', name: '__apm$args' } }] fn.async = operator === 'tracePromise' + state.selfBinding = null fn.body = wrap(state, { type: 'Identifier', name: savedBinding }, program) wrapSuper(state, fn) @@ -361,7 +377,7 @@ function traceInstanceMethod (state, node, program) { * @returns {import('estree').BlockStatement['body']} */ function wrap (state, node, program) { - const { operator, moduleVersion } = state + const { operator, moduleVersion, selfBinding } = state const { returnKind } = state.functionQuery const iterPatch = returnKind ? generateIterPatch(state, returnKind, program) : '' @@ -378,7 +394,10 @@ function wrap (state, node, program) { .map((_, i) => `__apm$arg${i}`).concat('...__apm$args').join(', ') const block = wrapper.body[0].body // Extract only block statement of function body. - const common = parse(node.type === 'ArrowFunctionExpression' + // Declared in the outer function scope so that the `super()` call sites in + // the moved body can write to it and the `finally` block can read it back. + const declareSelf = selfBinding ? `let ${selfBinding};` : '' + const common = parse(declareSelf + (node.type === 'ArrowFunctionExpression' ? ` const __apm$arguments = [${args}]; const __apm$ctx = { @@ -401,7 +420,7 @@ function wrap (state, node, program) { const __apm$wrapped = () => {}; return __apm$wrapped.apply(this, __apm$arguments); }; - `).body + `)).body block.body.unshift(...common) @@ -411,6 +430,75 @@ function wrap (state, node, program) { return block } +/** + * The expression a wrapper's `finally` block uses to fill in `message.self`. + * + * Most functions read `this` directly. A derived constructor cannot, so + * {@link captureSelfAtSuper} records `this` in a variable and the wrapper + * reads that variable instead. + * + * @param {{ selfBinding?: string|null }} state + * @returns {string} + */ +const selfExpression = ({ selfBinding }) => selfBinding || 'this' + +/** + * Rewrites each `super(...)` call in a constructor body so that it also + * records `this` in `binding`. + * + * A derived constructor's `this` stays unbound until `super()` returns, and + * the wrapper moves the original body into a nested arrow, so `super()` now + * runs while the wrapper's own `runStores` callback is on the stack. + * JavaScriptCore (Bun) loads `this` once, when a closure is entered, so that + * callback keeps the unbound value and throws when the `finally` block reads + * it. Assigning at the call site puts the read immediately after `super()`, + * where every engine agrees `this` is bound. + * + * `super(...)` already evaluates to the newly bound `this`, so the sequence + * expression returns the same value the call did. + * + * Nested classes and nested non-arrow functions are skipped: a `super()` call + * inside one of those belongs to that function, not to this constructor. + * + * @param {import('estree').Function} node - The constructor being wrapped. + * @param {string} binding - Name of the variable to record `this` in. + * @returns {boolean} `true` if at least one call was rewritten. + */ +function captureSelfAtSuper (node, binding) { + let found = false + + const visit = (parent, key) => { + const child = parent[key] + + if (child === null || typeof child !== 'object') return + + if (Array.isArray(child)) { + for (let i = 0; i < child.length; i++) visit(child, i) + return + } + + if (typeof child.type !== 'string') return + + if (child.type === 'ClassBody' || + child.type === 'FunctionExpression' || + child.type === 'FunctionDeclaration') return + + if (child.type === 'CallExpression' && child.callee.type === 'Super') { + const sequence = parse(`(0, ${binding} = this)`).body[0].expression + sequence.expressions[0] = child + parent[key] = sequence + found = true + return + } + + for (const name of Object.keys(child)) visit(child, name) + } + + visit(node, 'body') + + return found +} + /** * Rewrites `super.method(...)` calls inside a moved function body. * @@ -566,7 +654,7 @@ function wrapCallback (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } }); @@ -651,7 +739,7 @@ function wrapPromise (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } }); @@ -693,7 +781,7 @@ function wrapSync (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } return __apm$ctx.result; diff --git a/tests/constructor_self_cjs/mod.js b/tests/constructor_self_cjs/mod.js new file mode 100644 index 0000000..9ebd8ce --- /dev/null +++ b/tests/constructor_self_cjs/mod.js @@ -0,0 +1,19 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +class UndiciBase { + constructor (val) { + this.base = val + } +} + +class Undici extends UndiciBase { + constructor (val) { + if (val === 'boom') throw new Error('boom') + super(val) + this.val = val + } +} + +module.exports = Undici diff --git a/tests/constructor_self_cjs/test.js b/tests/constructor_self_cjs/test.js new file mode 100644 index 0000000..1a8f8e1 --- /dev/null +++ b/tests/constructor_self_cjs/test.js @@ -0,0 +1,30 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +const Undici = require('./instrumented.js') +const assert = require('node:assert') +const { tracingChannel } = require('node:diagnostics_channel') + +const ends = [] +tracingChannel('orchestrion:undici:Undici_constructor').subscribe({ + end (message) { + ends.push(message) + } +}) + +const undici = new Undici(42) +assert.strictEqual(undici.val, 42) +assert.strictEqual(undici.base, 42) +assert.strictEqual(ends.length, 1) +assert.strictEqual(ends[0].self, undici) + +// A throw before `super()` leaves `this` unbound, so the wrapper must not read +// it. If it does, the original error is replaced by a ReferenceError. +assert.throws(() => new Undici('boom'), (err) => { + assert.strictEqual(err.constructor, Error) + assert.strictEqual(err.message, 'boom') + return true +}) +assert.strictEqual(ends.length, 2) +assert.strictEqual(ends[1].self, undefined) diff --git a/tests/constructor_super_method_cjs/mod.js b/tests/constructor_super_method_cjs/mod.js new file mode 100644 index 0000000..6c784e9 --- /dev/null +++ b/tests/constructor_super_method_cjs/mod.js @@ -0,0 +1,22 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +class UndiciBase { + constructor (val) { + this.base = val + } + + greet () { + return `hi ${this.base}` + } +} + +class Undici extends UndiciBase { + constructor (val) { + super(val) + this.val = super.greet() + } +} + +module.exports = Undici diff --git a/tests/constructor_super_method_cjs/test.js b/tests/constructor_super_method_cjs/test.js new file mode 100644 index 0000000..828c2bf --- /dev/null +++ b/tests/constructor_super_method_cjs/test.js @@ -0,0 +1,19 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +const Undici = require('./instrumented.js') +const { assert, getContext } = require('../common/preamble.js') +const context = getContext('orchestrion:undici:Undici_constructor'); + +// `super.greet()` uses `this` as the receiver, so it only works after the +// body's own `super()` call. The wrapper must leave it where it was. +(() => { + const undici = new Undici(42) + assert.strictEqual(undici.base, 42) + assert.strictEqual(undici.val, 'hi 42') + assert.deepStrictEqual(context, { + start: true, + end: true + }) +})() diff --git a/tests/tests.test.mjs b/tests/tests.test.mjs index 6db2b7b..a0ce642 100644 --- a/tests/tests.test.mjs +++ b/tests/tests.test.mjs @@ -10,6 +10,20 @@ import { SourceMapConsumer } from 'source-map' const __dirname = dirname(fileURLToPath(import.meta.url)) +// Bun is not an officially supported engine, but people do run this code under +// it, and its JavaScriptCore engine is stricter than V8 about reading `this` in +// a derived constructor. Run every fixture under Bun as well when it is +// installed, so that engine's rules stay covered. +const hasBun = spawnSync('bun', ['--version'], { stdio: 'ignore' }).status === 0 + +function runEngine (engine, file, cwd) { + const result = spawnSync(engine, [file], { cwd, stdio: 'pipe' }) + if (result.status !== 0) { + const output = (result.stdout?.toString() || '') + (result.stderr?.toString() || '') + throw new Error(`${engine} ${file} exited with ${result.status}:\n${output}`) + } +} + const TEST_MODULE_NAME = 'undici' const TEST_MODULE_VERSION = '0.0.1' const TEST_MODULE_PATH = 'index.mjs' @@ -41,12 +55,8 @@ function runTest (testName, configs, { mjs = false, filePath = TEST_MODULE_PATH, // Injection failure — do not write instrumented file } - const result = spawnSync('node', [`test.${ext}`], { cwd: testDir, stdio: 'pipe' }) - if (result.status !== 0) { - const output = (result.stdout?.toString() || '') + (result.stderr?.toString() || '') - throw new Error(`node test.${ext} exited with ${result.status}:\n${output}`) - } - assert.equal(result.status, 0) + runEngine('node', `test.${ext}`, testDir) + if (hasBun) runEngine('bun', `test.${ext}`, testDir) } describe('arguments_mutation', () => { @@ -114,6 +124,30 @@ describe('constructor_cjs', () => { }) }) +describe('constructor_super_method_cjs', () => { + test('keeps super.method() working inside a derived constructor', () => { + runTest('constructor_super_method_cjs', [ + { + channelName: 'Undici_constructor', + module: { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH }, + functionQuery: { className: 'Undici' }, + }, + ]) + }) +}) + +describe('constructor_self_cjs', () => { + test('reports the instance as message.self and does not mask a throw before super()', () => { + runTest('constructor_self_cjs', [ + { + channelName: 'Undici_constructor', + module: { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH }, + functionQuery: { className: 'Undici' }, + }, + ]) + }) +}) + describe('constructor_mjs', () => { test('instruments class constructor (mjs)', () => { runTest('constructor_mjs', [