Skip to content

Commit 9a8c0b6

Browse files
jriekenjrieken
authored
Merge pull request microsoft#184932 from thecrypticace/fix/no-prepare-rewrapping
Don’t re-wrap `prepareStackTrace` when assigned back to itself
2 parents b4e06ef + 498bf5c commit 9a8c0b6

File tree

2 files changed

+252
-11
lines changed

2 files changed

+252
-11
lines changed

‎src/vs/workbench/api/common/extensionHostMain.ts

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export interface IConsolePatchFn {
3232
(mainThreadConsole: MainThreadConsoleShape): any;
3333
}
3434

35-
abstract class ErrorHandler {
35+
export abstract class ErrorHandler {
3636

3737
static {
3838
// increase number of stack frames (from 10, https://github.com/v8/v8/wiki/Stack-Trace-API)
@@ -65,12 +65,15 @@ abstract class ErrorHandler {
6565
const mainThreadErrors = rpcService.getProxy(MainContext.MainThreadErrors);
6666

6767
const map = await extensionService.getExtensionPathIndex();
68-
const extensionErrors = new WeakMap<Error, ExtensionIdentifier | undefined>();
68+
const extensionErrors = new WeakMap<Error, { extensionIdentifier: ExtensionIdentifier | undefined; stack: string }>();
6969

7070
// PART 1
7171
// set the prepareStackTrace-handle and use it as a side-effect to associate errors
7272
// with extensions - this works by looking up callsites in the extension path index
7373
function prepareStackTraceAndFindExtension(error: Error, stackTrace: errors.V8CallSite[]) {
74+
if (extensionErrors.has(error)) {
75+
return extensionErrors.get(error)!.stack;
76+
}
7477
let stackTraceMessage = '';
7578
let extension: IExtensionDescription | undefined;
7679
let fileName: string | null;
@@ -81,21 +84,31 @@ abstract class ErrorHandler {
8184
extension = map.findSubstr(URI.file(fileName));
8285
}
8386
}
84-
extensionErrors.set(error, extension?.identifier);
85-
return `${error.name || 'Error'}: ${error.message || ''}${stackTraceMessage}`;
87+
const result = `${error.name || 'Error'}: ${error.message || ''}${stackTraceMessage}`;
88+
extensionErrors.set(error, { extensionIdentifier: extension?.identifier, stack: result });
89+
return result;
8690
}
8791

92+
const _wasWrapped = Symbol('prepareStackTrace wrapped');
8893
let _prepareStackTrace = prepareStackTraceAndFindExtension;
94+
8995
Object.defineProperty(Error, 'prepareStackTrace', {
9096
configurable: false,
9197
get() {
9298
return _prepareStackTrace;
9399
},
94100
set(v) {
101+
if (v === prepareStackTraceAndFindExtension || v[_wasWrapped]) {
102+
_prepareStackTrace = v;
103+
return;
104+
}
105+
95106
_prepareStackTrace = function (error, stackTrace) {
96107
prepareStackTraceAndFindExtension(error, stackTrace);
97108
return v.call(Error, error, stackTrace);
98109
};
110+
111+
Object.assign(_prepareStackTrace, { [_wasWrapped]: true });
99112
},
100113
});
101114

@@ -106,16 +119,16 @@ abstract class ErrorHandler {
106119
errors.setUnexpectedErrorHandler(err => {
107120
logService.error(err);
108121

109-
const data = errors.transformErrorForSerialization(err);
110-
const extension = extensionErrors.get(err);
111-
if (!extension) {
112-
mainThreadErrors.$onUnexpectedError(data);
122+
const errorData = errors.transformErrorForSerialization(err);
123+
const stackData = extensionErrors.get(err);
124+
if (!stackData?.extensionIdentifier) {
125+
mainThreadErrors.$onUnexpectedError(errorData);
113126
return;
114127
}
115128

116-
mainThreadExtensions.$onExtensionRuntimeError(extension, data);
117-
const reported = extensionTelemetry.onExtensionError(extension, err);
118-
logService.trace('forwarded error to extension?', reported, extension);
129+
mainThreadExtensions.$onExtensionRuntimeError(stackData.extensionIdentifier, errorData);
130+
const reported = extensionTelemetry.onExtensionError(stackData.extensionIdentifier, err);
131+
logService.trace('forwarded error to extension?', reported, stackData);
119132
});
120133
}
121134
}

‎src/vs/workbench/api/test/common/extensionHostMain.test.ts

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import * as assert from 'assert';
7+
import { SerializedError, errorHandler, onUnexpectedError } from 'vs/base/common/errors';
8+
import { TernarySearchTree } from 'vs/base/common/ternarySearchTree';
9+
import { URI } from 'vs/base/common/uri';
10+
import { mock } from 'vs/base/test/common/mock';
11+
import { ExtensionIdentifier, IExtensionDescription, IRelaxedExtensionDescription } from 'vs/platform/extensions/common/extensions';
12+
import { InstantiationService } from 'vs/platform/instantiation/common/instantiationService';
13+
import { ServiceCollection } from 'vs/platform/instantiation/common/serviceCollection';
14+
import { ILogService, NullLogService } from 'vs/platform/log/common/log';
15+
import { MainThreadExtensionServiceShape } from 'vs/workbench/api/common/extHost.protocol';
16+
import { ExtensionPaths, IExtHostExtensionService } from 'vs/workbench/api/common/extHostExtensionService';
17+
import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService';
18+
import { IExtHostTelemetry } from 'vs/workbench/api/common/extHostTelemetry';
19+
import { ErrorHandler } from 'vs/workbench/api/common/extensionHostMain';
20+
import { nullExtensionDescription } from 'vs/workbench/services/extensions/common/extensions';
21+
import { ProxyIdentifier, Proxied } from 'vs/workbench/services/extensions/common/proxyIdentifier';
22+
23+
24+
suite('ExtensionHostMain#ErrorHandler - Wrapping prepareStackTrace can cause slowdown and eventual stack overflow #184926 ', function () {
25+
26+
const extensionsIndex = TernarySearchTree.forUris<IExtensionDescription>();
27+
const mainThreadExtensionsService = new class extends mock<MainThreadExtensionServiceShape>() {
28+
override $onExtensionRuntimeError(extensionId: ExtensionIdentifier, data: SerializedError): void {
29+
30+
}
31+
};
32+
33+
const collection = new ServiceCollection(
34+
[ILogService, new NullLogService()],
35+
[IExtHostTelemetry, new class extends mock<IExtHostTelemetry>() {
36+
declare readonly _serviceBrand: undefined;
37+
override onExtensionError(extension: ExtensionIdentifier, error: Error): boolean {
38+
return true;
39+
}
40+
}],
41+
[IExtHostExtensionService, new class extends mock<IExtHostExtensionService & any>() {
42+
declare readonly _serviceBrand: undefined;
43+
getExtensionPathIndex() {
44+
return new class extends ExtensionPaths {
45+
override findSubstr(key: URI): Readonly<IRelaxedExtensionDescription> | undefined {
46+
findSubstrCount++;
47+
return nullExtensionDescription;
48+
}
49+
50+
}(extensionsIndex);
51+
}
52+
}],
53+
[IExtHostRpcService, new class extends mock<IExtHostRpcService>() {
54+
declare readonly _serviceBrand: undefined;
55+
override getProxy<T>(identifier: ProxyIdentifier<T>): Proxied<T> {
56+
return <any>mainThreadExtensionsService;
57+
}
58+
}]
59+
);
60+
61+
const insta = new InstantiationService(collection, false);
62+
63+
let existingErrorHandler: (e: any) => void;
64+
let findSubstrCount = 0;
65+
66+
suiteSetup(async function () {
67+
existingErrorHandler = errorHandler.getUnexpectedErrorHandler();
68+
await insta.invokeFunction(ErrorHandler.installFullHandler);
69+
});
70+
71+
suiteTeardown(function () {
72+
errorHandler.setUnexpectedErrorHandler(existingErrorHandler);
73+
});
74+
75+
setup(async function () {
76+
findSubstrCount = 0;
77+
});
78+
79+
test('basics', function () {
80+
81+
const err = new Error('test1');
82+
83+
onUnexpectedError(err);
84+
85+
assert.strictEqual(findSubstrCount, 1);
86+
87+
});
88+
89+
test('set/reset prepareStackTrace-callback', function () {
90+
91+
const original = Error.prepareStackTrace;
92+
Error.prepareStackTrace = (_error, _stack) => 'stack';
93+
const probeErr = new Error();
94+
const stack = probeErr.stack;
95+
assert.ok(stack);
96+
Error.prepareStackTrace = original;
97+
assert.strictEqual(findSubstrCount, 1);
98+
99+
// already checked
100+
onUnexpectedError(probeErr);
101+
assert.strictEqual(findSubstrCount, 1);
102+
103+
// one more error
104+
const err = new Error('test2');
105+
onUnexpectedError(err);
106+
107+
assert.strictEqual(findSubstrCount, 2);
108+
});
109+
110+
test('wrap prepareStackTrace-callback', function () {
111+
112+
function do_something_else(params: string) {
113+
return params;
114+
}
115+
116+
const original = Error.prepareStackTrace;
117+
Error.prepareStackTrace = (...args) => {
118+
return do_something_else(original?.(...args));
119+
};
120+
const probeErr = new Error();
121+
const stack = probeErr.stack;
122+
assert.ok(stack);
123+
124+
125+
onUnexpectedError(probeErr);
126+
assert.strictEqual(findSubstrCount, 1);
127+
});
128+
129+
test('prevent rewrapping', function () {
130+
131+
let do_something_count = 0;
132+
function do_something(params: any) {
133+
do_something_count++;
134+
}
135+
136+
Error.prepareStackTrace = (result, stack) => {
137+
do_something(stack);
138+
return 'fakestack';
139+
};
140+
141+
for (let i = 0; i < 2_500; ++i) {
142+
Error.prepareStackTrace = Error.prepareStackTrace;
143+
}
144+
145+
const probeErr = new Error();
146+
const stack = probeErr.stack;
147+
assert.strictEqual(stack, 'fakestack');
148+
149+
onUnexpectedError(probeErr);
150+
assert.strictEqual(findSubstrCount, 1);
151+
152+
const probeErr2 = new Error();
153+
onUnexpectedError(probeErr2);
154+
assert.strictEqual(findSubstrCount, 2);
155+
assert.strictEqual(do_something_count, 2);
156+
});
157+
158+
159+
suite('https://gist.github.com/thecrypticace/f0f2e182082072efdaf0f8e1537d2cce', function () {
160+
161+
test("Restored, separate operations", () => {
162+
// Actual Test
163+
let original;
164+
165+
// Operation 1
166+
original = Error.prepareStackTrace;
167+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
168+
const err1 = new Error();
169+
assert.ok(err1.stack);
170+
assert.strictEqual(findSubstrCount, 1);
171+
Error.prepareStackTrace = original;
172+
173+
// Operation 2
174+
original = Error.prepareStackTrace;
175+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
176+
assert.ok(new Error().stack);
177+
assert.strictEqual(findSubstrCount, 2);
178+
Error.prepareStackTrace = original;
179+
180+
// Operation 3
181+
original = Error.prepareStackTrace;
182+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
183+
assert.ok(new Error().stack);
184+
assert.strictEqual(findSubstrCount, 3);
185+
Error.prepareStackTrace = original;
186+
187+
// Operation 4
188+
original = Error.prepareStackTrace;
189+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
190+
assert.ok(new Error().stack);
191+
assert.strictEqual(findSubstrCount, 4);
192+
Error.prepareStackTrace = original;
193+
194+
// Back to Operation 1
195+
assert.ok(err1.stack);
196+
assert.strictEqual(findSubstrCount, 4);
197+
});
198+
199+
test("Never restored, separate operations", () => {
200+
// Operation 1
201+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
202+
assert.ok(new Error().stack);
203+
204+
// Operation 2
205+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
206+
assert.ok(new Error().stack);
207+
208+
// Operation 3
209+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
210+
assert.ok(new Error().stack);
211+
212+
// Operation 4
213+
for (let i = 0; i < 12_500; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
214+
assert.ok(new Error().stack);
215+
});
216+
217+
test("Restored, too many uses before restoration", async () => {
218+
const original = Error.prepareStackTrace;
219+
Error.prepareStackTrace = (_, stack) => stack;
220+
221+
// Operation 1 — more uses of `prepareStackTrace`
222+
for (let i = 0; i < 10_000; ++i) { Error.prepareStackTrace = Error.prepareStackTrace; }
223+
assert.ok(new Error().stack);
224+
225+
Error.prepareStackTrace = original;
226+
});
227+
});
228+
});

0 commit comments

Comments
 (0)