Merge pull request #95 from webpack/bugfix/stack-overflow

Avoid stack overflow while series hook compilation
webpack · Apr 12, 2019 · 32afdc5 · 32afdc5
2 parents f12a34f + a83eda2
commit 32afdc5
Show file tree

Hide file tree

Showing 9 changed files with 1,286 additions and 93 deletions.
diff --git a/lib/AsyncSeriesBailHook.js b/lib/AsyncSeriesBailHook.js
@@ -8,13 +8,14 @@ const Hook = require("./Hook");
 const HookCodeFactory = require("./HookCodeFactory");
 
 class AsyncSeriesBailHookCodeFactory extends HookCodeFactory {
-	content({ onError, onResult, onDone }) {
+	content({ onError, onResult, resultReturns, onDone }) {
 		return this.callTapsSeries({
 			onError: (i, err, next, doneBreak) => onError(err) + doneBreak(true),
 			onResult: (i, result, next) =>
 				`if(${result} !== undefined) {\n${onResult(
 					result
 				)};\n} else {\n${next()}}\n`,
+			resultReturns,
 			onDone
 		});
 	}

diff --git a/lib/HookCodeFactory.js b/lib/HookCodeFactory.js
@@ -23,6 +23,7 @@ class HookCodeFactory {
 						this.content({
 							onError: err => `throw ${err};\n`,
 							onResult: result => `return ${result};\n`,
+							resultReturns: true,
 							onDone: () => "",
 							rethrowIfPossible: true
 						})
@@ -43,24 +44,32 @@ class HookCodeFactory {
 				);
 				break;
 			case "promise":
-				let code = "";
-				code += '"use strict";\n';
-				code += "return new Promise((_resolve, _reject) => {\n";
-				code += "var _sync = true;\n";
-				code += this.header();
-				code += this.content({
+				let errorHelperUsed = false;
+				const content = this.content({
 					onError: err => {
-						let code = "";
-						code += "if(_sync)\n";
-						code += `_resolve(Promise.resolve().then(() => { throw ${err}; }));\n`;
-						code += "else\n";
-						code += `_reject(${err});\n`;
-						return code;
+						errorHelperUsed = true;
+						return `_error(${err});\n`;
 					},
 					onResult: result => `_resolve(${result});\n`,
 					onDone: () => "_resolve();\n"
 				});
-				code += "_sync = false;\n";
+				let code = "";
+				code += '"use strict";\n';
+				code += "return new Promise((_resolve, _reject) => {\n";
+				if (errorHelperUsed) {
+					code += "var _sync = true;\n";
+					code += "function _error(_err) {\n";
+					code += "if(_sync)\n";
+					code += "_resolve(Promise.resolve().then(() => { throw _err; }));\n";
+					code += "else\n";
+					code += "_reject(_err);\n";
+					code += "};\n";
+				}
+				code += this.header();
+				code += content;
+				if (errorHelperUsed) {
+					code += "_sync = false;\n";
+				}
 				code += "});\n";
 				fn = new Function(this.args(), code);
 				break;
@@ -207,35 +216,48 @@ class HookCodeFactory {
 		return code;
 	}
 
-	callTapsSeries({ onError, onResult, onDone, rethrowIfPossible }) {
+	callTapsSeries({
+		onError,
+		onResult,
+		resultReturns,
+		onDone,
+		doneReturns,
+		rethrowIfPossible
+	}) {
 		if (this.options.taps.length === 0) return onDone();
 		const firstAsync = this.options.taps.findIndex(t => t.type !== "sync");
-		const next = i => {
-			if (i >= this.options.taps.length) {
-				return onDone();
+		const somethingReturns = resultReturns || doneReturns || false;
+		let code = "";
+		let current = onDone;
+		for (let j = this.options.taps.length - 1; j >= 0; j--) {
+			const i = j;
+			const unroll = current !== onDone && this.options.taps[i].type !== "sync";
+			if (unroll) {
+				code += `function _next${i}() {\n`;
+				code += current();
+				code += `}\n`;
+				current = () => `${somethingReturns ? "return " : ""}_next${i}();\n`;
 			}
-			const done = () => next(i + 1);
+			const done = current;
 			const doneBreak = skipDone => {
 				if (skipDone) return "";
 				return onDone();
 			};
-			return this.callTap(i, {
+			const content = this.callTap(i, {
 				onError: error => onError(i, error, done, doneBreak),
 				onResult:
 					onResult &&
 					(result => {
 						return onResult(i, result, done, doneBreak);
 					}),
-				onDone:
-					!onResult &&
-					(() => {
-						return done();
-					}),
+				onDone: !onResult && done,
 				rethrowIfPossible:
 					rethrowIfPossible && (firstAsync < 0 || i < firstAsync)
 			});
-		};
-		return next(0);
+			current = () => content;
+		}
+		code += current();
+		return code;
 	}
 
 	callTapsLooping({ onError, onDone, rethrowIfPossible }) {

diff --git a/lib/SyncBailHook.js b/lib/SyncBailHook.js
@@ -8,13 +8,14 @@ const Hook = require("./Hook");
 const HookCodeFactory = require("./HookCodeFactory");
 
 class SyncBailHookCodeFactory extends HookCodeFactory {
-	content({ onError, onResult, onDone, rethrowIfPossible }) {
+	content({ onError, onResult, resultReturns, onDone, rethrowIfPossible }) {
 		return this.callTapsSeries({
 			onError: (i, err) => onError(err),
 			onResult: (i, result, next) =>
 				`if(${result} !== undefined) {\n${onResult(
 					result
 				)};\n} else {\n${next()}}\n`,
+			resultReturns,
 			onDone,
 			rethrowIfPossible
 		});

diff --git a/lib/SyncHook.js b/lib/SyncHook.js
@@ -8,7 +8,7 @@ const Hook = require("./Hook");
 const HookCodeFactory = require("./HookCodeFactory");
 
 class SyncHookCodeFactory extends HookCodeFactory {
-	content({ onError, onResult, onDone, rethrowIfPossible }) {
+	content({ onError, onDone, rethrowIfPossible }) {
 		return this.callTapsSeries({
 			onError: (i, err) => onError(err),
 			onDone,

diff --git a/lib/SyncLoopHook.js b/lib/SyncLoopHook.js
@@ -8,7 +8,7 @@ const Hook = require("./Hook");
 const HookCodeFactory = require("./HookCodeFactory");
 
 class SyncLoopHookCodeFactory extends HookCodeFactory {
-	content({ onError, onResult, onDone, rethrowIfPossible }) {
+	content({ onError, onDone, rethrowIfPossible }) {
 		return this.callTapsLooping({
 			onError: (i, err) => onError(err),
 			onDone,

diff --git a/lib/SyncWaterfallHook.js b/lib/SyncWaterfallHook.js
@@ -8,7 +8,7 @@ const Hook = require("./Hook");
 const HookCodeFactory = require("./HookCodeFactory");
 
 class SyncWaterfallHookCodeFactory extends HookCodeFactory {
-	content({ onError, onResult, onDone, rethrowIfPossible }) {
+	content({ onError, onResult, resultReturns, rethrowIfPossible }) {
 		return this.callTapsSeries({
 			onError: (i, err) => onError(err),
 			onResult: (i, result, next) => {
@@ -20,6 +20,7 @@ class SyncWaterfallHookCodeFactory extends HookCodeFactory {
 				return code;
 			},
 			onDone: () => onResult(this._args[0]),
+			doneReturns: resultReturns,
 			rethrowIfPossible
 		});
 	}

diff --git a/lib/__tests__/HookCodeFactory.js b/lib/__tests__/HookCodeFactory.js
@@ -10,6 +10,10 @@ const expectNoSyntaxError = code => {
 	new Function("a, b, c", code);
 };
 
+const pretty = code => {
+	return require("prettier").format(code, { filepath: "code.js" });
+};
+
 describe("HookCodeFactory", () => {
 	describe("callTap", () => {
 		const factoryConfigurations = {
@@ -83,6 +87,7 @@ describe("HookCodeFactory", () => {
 						onDone: () => "onDone();\n"
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("sync with onResult", () => {
@@ -91,6 +96,7 @@ describe("HookCodeFactory", () => {
 						onResult: result => `onResult(${result});\n`
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("async without onResult", () => {
@@ -99,6 +105,7 @@ describe("HookCodeFactory", () => {
 						onDone: () => "onDone();\n"
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("async with onResult", () => {
@@ -107,6 +114,7 @@ describe("HookCodeFactory", () => {
 						onResult: result => `onResult(${result});\n`
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("promise without onResult", () => {
@@ -115,6 +123,7 @@ describe("HookCodeFactory", () => {
 						onDone: () => "onDone();\n"
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("promise with onResult", () => {
@@ -123,6 +132,7 @@ describe("HookCodeFactory", () => {
 						onResult: result => `onResult(${result});\n`
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 			});
@@ -224,6 +234,7 @@ describe("HookCodeFactory", () => {
 						rethrowIfPossible: true
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("callTapsParallel", () => {
@@ -235,6 +246,7 @@ describe("HookCodeFactory", () => {
 						rethrowIfPossible: true
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 				it("callTapsLooping", () => {
@@ -244,6 +256,7 @@ describe("HookCodeFactory", () => {
 						rethrowIfPossible: true
 					});
 					expect(code).toMatchSnapshot();
+					expect(pretty(code)).toMatchSnapshot();
 					expectNoSyntaxError(code);
 				});
 			});

diff --git a/lib/__tests__/HookStackOverflow.js b/lib/__tests__/HookStackOverflow.js
@@ -0,0 +1,15 @@
+const AsyncSeriesHook = require("../AsyncSeriesHook");
+
+describe("HookStackOverflow", () => {
+	it("should not crash when compiling a large hook", () => {
+		const hook = new AsyncSeriesHook(["a", "b"]);
+
+		for (let i = 0; i < 10; i++) {
+			hook.tap("TestPlugin", (a, b) => {});
+			hook.tapAsync("TestPlugin", (a, b, callback) => callback());
+			hook.tapPromise("TestPlugin", (a, b) => Promise.resolve());
+		}
+
+		return hook.promise(1, 2);
+	});
+});