Merge pull request #13837 from webpack/bugfix/13827

fix problem when updating persistent cached modules from unsafe cached modules
webpack · Jul 22, 2021 · e83587c · e83587c
2 parents e293d2b + c23e8ce
commit e83587c
Show file tree

Hide file tree

Showing 12 changed files with 159 additions and 80 deletions.
diff --git a/lib/NormalModule.js b/lib/NormalModule.js
@@ -350,18 +350,20 @@ class NormalModule extends Module {
 		this.resource = m.resource;
 		this.matchResource = m.matchResource;
 		this.loaders = m.loaders;
-		this._sourceTypes = m._sourceTypes;
-		this._sourceSizes = m._sourceSizes;
 	}
 
 	/**
 	 * Assuming this module is in the cache. Remove internal references to allow freeing some memory.
 	 */
 	cleanupForCache() {
-		// Make sure to cache types and sizes before cleanup
-		if (this._sourceTypes === undefined) this.getSourceTypes();
-		for (const type of this._sourceTypes) {
-			this.size(type);
+		// Make sure to cache types and sizes before cleanup when this module has been built
+		// They are accessed by the stats and we don't want them to crash after cleanup
+		// TODO reconsider this for webpack 6
+		if (this.buildInfo) {
+			if (this._sourceTypes === undefined) this.getSourceTypes();
+			for (const type of this._sourceTypes) {
+				this.size(type);
+			}
 		}
 		super.cleanupForCache();
 		this.parser = undefined;

diff --git a/lib/asset/AssetModulesPlugin.js b/lib/asset/AssetModulesPlugin.js
@@ -172,24 +172,29 @@ class AssetModulesPlugin {
 					);
 					if (modules) {
 						for (const module of modules) {
-							const codeGenResult = codeGenerationResults.get(
-								module,
-								chunk.runtime
-							);
-							result.push({
-								render: () => codeGenResult.sources.get(type),
-								filename:
-									module.buildInfo.filename ||
-									codeGenResult.data.get("filename"),
-								info:
-									module.buildInfo.assetInfo ||
-									codeGenResult.data.get("assetInfo"),
-								auxiliary: true,
-								identifier: `assetModule${chunkGraph.getModuleId(module)}`,
-								hash:
-									module.buildInfo.fullContentHash ||
-									codeGenResult.data.get("fullContentHash")
-							});
+							try {
+								const codeGenResult = codeGenerationResults.get(
+									module,
+									chunk.runtime
+								);
+								result.push({
+									render: () => codeGenResult.sources.get(type),
+									filename:
+										module.buildInfo.filename ||
+										codeGenResult.data.get("filename"),
+									info:
+										module.buildInfo.assetInfo ||
+										codeGenResult.data.get("assetInfo"),
+									auxiliary: true,
+									identifier: `assetModule${chunkGraph.getModuleId(module)}`,
+									hash:
+										module.buildInfo.fullContentHash ||
+										codeGenResult.data.get("fullContentHash")
+								});
+							} catch (e) {
+								e.message += `\nduring rendering of asset ${module.identifier()}`;
+								throw e;
+							}
 						}
 					}
 

diff --git a/test/ConfigTestCases.template.js b/test/ConfigTestCases.template.js
@@ -235,7 +235,7 @@ const describeCases = config => {
 							rimraf.sync(outputDirectory);
 							fs.mkdirSync(outputDirectory, { recursive: true });
 							const deprecationTracker = deprecationTracking.start();
-							require("..")(options, (err, stats) => {
+							const onCompiled = (err, stats) => {
 								const deprecations = deprecationTracker();
 								if (err) return handleFatalError(err, done);
 								const statOptions = {
@@ -570,7 +570,25 @@ const describeCases = config => {
 										done();
 									})
 									.catch(done);
-							});
+							};
+							if (config.cache) {
+								try {
+									const compiler = require("..")(options);
+									compiler.run(err => {
+										if (err) return handleFatalError(err, done);
+										compiler.run((error, stats) => {
+											compiler.close(err => {
+												if (err) return handleFatalError(err, done);
+												onCompiled(error, stats);
+											});
+										});
+									});
+								} catch (e) {
+									handleFatalError(e, done);
+								}
+							} else {
+								require("..")(options, onCompiled);
+							}
 						}, 30000);
 
 						const {

diff --git a/test/configCases/asset-modules/http-url/server/index.js b/test/configCases/asset-modules/http-url/server/index.js
@@ -6,7 +6,10 @@ const fs = require("fs");
  * @returns {Promise<import("http").Server>} server instance
  */
 function createServer(port) {
-	const file = fs.readFileSync("./test/configCases/asset-modules/http-url/server/index.css").toString().trim();
+	const file = fs
+		.readFileSync("./test/configCases/asset-modules/http-url/server/index.css")
+		.toString()
+		.trim();
 
 	const server = http.createServer((req, res) => {
 		if (req.url !== "/index.css") {
@@ -18,7 +21,7 @@ function createServer(port) {
 	});
 
 	return new Promise((resolve, reject) => {
-		server.listen(port, (err) => {
+		server.listen(port, err => {
 			if (err) {
 				reject(err);
 			} else {
@@ -40,21 +43,25 @@ class ServerPlugin {
 	 * @param {import("../../../../../").Compiler} compiler
 	 */
 	apply(compiler) {
-		const serverPromise = createServer(this.port);
+		let server;
 
-		serverPromise
-			.then(server => server.unref());
+		compiler.hooks.beforeRun.tapPromise(
+			"ServerPlugin",
+			async (compiler, callback) => {
+				if (!server) {
+					server = await createServer(this.port);
+					server.unref();
+				}
+			}
+		);
 
 		compiler.hooks.done.tapAsync("ServerPlugin", (stats, callback) => {
-			serverPromise
-				.then(server => server.close(callback))
-				.catch(callback)
-		});
-
-		compiler.hooks.beforeRun.tapAsync("ServerPlugin", (compiler, callback) => {
-			serverPromise
-				.then(() => callback())
-				.catch(callback)
+			if (server) {
+				server.close(callback);
+				server = undefined;
+			} else {
+				callback();
+			}
 		});
 	}
 }

diff --git a/test/configCases/asset-modules/unsafe-cache-13827/index.js b/test/configCases/asset-modules/unsafe-cache-13827/index.js
@@ -0,0 +1,5 @@
+import url from "package";
+
+it("should create a data url", () => {
+	expect(url.protocol).toBe("data:");
+});
diff --git a/test/configCases/asset-modules/unsafe-cache-13827/node_modules/package/file.svg b/test/configCases/asset-modules/unsafe-cache-13827/node_modules/package/file.svg
diff --git a/test/configCases/asset-modules/unsafe-cache-13827/node_modules/package/index.js b/test/configCases/asset-modules/unsafe-cache-13827/node_modules/package/index.js
diff --git a/test/configCases/asset-modules/unsafe-cache-13827/webpack.config.js b/test/configCases/asset-modules/unsafe-cache-13827/webpack.config.js
@@ -0,0 +1,12 @@
+/** @type {import("../../../../").Configuration} */
+module.exports = {
+	mode: "development",
+	module: {
+		rules: [
+			{
+				dependency: "url",
+				type: "asset"
+			}
+		]
+	}
+};
diff --git a/test/configCases/clean/enabled/webpack.config.js b/test/configCases/clean/enabled/webpack.config.js
@@ -10,12 +10,19 @@ module.exports = {
 	},
 	plugins: [
 		compiler => {
+			let once = true;
 			compiler.hooks.thisCompilation.tap("Test", compilation => {
 				compilation.hooks.processAssets.tap("Test", assets => {
-					const outputPath = compilation.getPath(compiler.outputPath, {});
-					const customDir = path.join(outputPath, "this/dir/should/be/removed");
-					fs.mkdirSync(customDir, { recursive: true });
-					fs.writeFileSync(path.join(customDir, "file.ext"), "");
+					if (once) {
+						const outputPath = compilation.getPath(compiler.outputPath, {});
+						const customDir = path.join(
+							outputPath,
+							"this/dir/should/be/removed"
+						);
+						fs.mkdirSync(customDir, { recursive: true });
+						fs.writeFileSync(path.join(customDir, "file.ext"), "");
+						once = false;
+					}
 					assets["this/dir/should/not/be/removed/file.ext"] = new RawSource("");
 				});
 			});

diff --git a/test/configCases/clean/ignore-fn/webpack.config.js b/test/configCases/clean/ignore-fn/webpack.config.js
@@ -14,18 +14,25 @@ module.exports = {
 	},
 	plugins: [
 		compiler => {
+			let once = true;
 			compiler.hooks.thisCompilation.tap("Test", compilation => {
 				compilation.hooks.processAssets.tap("Test", assets => {
-					const outputPath = compilation.getPath(compiler.outputPath, {});
-					const customDir = path.join(outputPath, "this/dir/should/be/removed");
-					const ignoredDir = path.join(
-						outputPath,
-						"this/is/ignored/dir/that/should/not/be/removed"
-					);
-					fs.mkdirSync(customDir, { recursive: true });
-					fs.writeFileSync(path.join(customDir, "file.ext"), "");
-					fs.mkdirSync(ignoredDir, { recursive: true });
-					fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
+					if (once) {
+						const outputPath = compilation.getPath(compiler.outputPath, {});
+						const customDir = path.join(
+							outputPath,
+							"this/dir/should/be/removed"
+						);
+						const ignoredDir = path.join(
+							outputPath,
+							"this/is/ignored/dir/that/should/not/be/removed"
+						);
+						fs.mkdirSync(customDir, { recursive: true });
+						fs.writeFileSync(path.join(customDir, "file.ext"), "");
+						fs.mkdirSync(ignoredDir, { recursive: true });
+						fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
+						once = false;
+					}
 					assets["this/dir/should/not/be/removed/file.ext"] = new RawSource("");
 				});
 			});

diff --git a/test/configCases/clean/ignore-hook/webpack.config.js b/test/configCases/clean/ignore-hook/webpack.config.js
@@ -11,6 +11,7 @@ module.exports = {
 	},
 	plugins: [
 		compiler => {
+			let once = true;
 			compiler.hooks.thisCompilation.tap("Test", compilation => {
 				webpack.CleanPlugin.getCompilationHooks(compilation).keep.tap(
 					"Test",
@@ -20,22 +21,28 @@ module.exports = {
 					}
 				);
 				compilation.hooks.processAssets.tap("Test", assets => {
-					const outputPath = compilation.getPath(compiler.outputPath, {});
-					const customDir = path.join(outputPath, "this/dir/should/be/removed");
-					const ignoredDir = path.join(
-						outputPath,
-						"this/is/ignored/dir/that/should/not/be/removed"
-					);
-					const ignoredTooDir = path.join(
-						outputPath,
-						"this/is/ignored/too/dir/that/should/not/be/removed"
-					);
-					fs.mkdirSync(customDir, { recursive: true });
-					fs.writeFileSync(path.join(customDir, "file.ext"), "");
-					fs.mkdirSync(ignoredDir, { recursive: true });
-					fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
-					fs.mkdirSync(ignoredTooDir, { recursive: true });
-					fs.writeFileSync(path.join(ignoredTooDir, "file.ext"), "");
+					if (once) {
+						const outputPath = compilation.getPath(compiler.outputPath, {});
+						const customDir = path.join(
+							outputPath,
+							"this/dir/should/be/removed"
+						);
+						const ignoredDir = path.join(
+							outputPath,
+							"this/is/ignored/dir/that/should/not/be/removed"
+						);
+						const ignoredTooDir = path.join(
+							outputPath,
+							"this/is/ignored/too/dir/that/should/not/be/removed"
+						);
+						fs.mkdirSync(customDir, { recursive: true });
+						fs.writeFileSync(path.join(customDir, "file.ext"), "");
+						fs.mkdirSync(ignoredDir, { recursive: true });
+						fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
+						fs.mkdirSync(ignoredTooDir, { recursive: true });
+						fs.writeFileSync(path.join(ignoredTooDir, "file.ext"), "");
+						once = false;
+					}
 					assets["this/dir/should/not/be/removed/file.ext"] = new RawSource("");
 				});
 			});

diff --git a/test/configCases/clean/ignore-rx/webpack.config.js b/test/configCases/clean/ignore-rx/webpack.config.js
@@ -12,18 +12,25 @@ module.exports = {
 	},
 	plugins: [
 		compiler => {
+			let once = true;
 			compiler.hooks.thisCompilation.tap("Test", compilation => {
 				compilation.hooks.processAssets.tap("Test", assets => {
-					const outputPath = compilation.getPath(compiler.outputPath, {});
-					const customDir = path.join(outputPath, "this/dir/should/be/removed");
-					const ignoredDir = path.join(
-						outputPath,
-						"this/is/ignored/dir/that/should/not/be/removed"
-					);
-					fs.mkdirSync(customDir, { recursive: true });
-					fs.writeFileSync(path.join(customDir, "file.ext"), "");
-					fs.mkdirSync(ignoredDir, { recursive: true });
-					fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
+					if (once) {
+						const outputPath = compilation.getPath(compiler.outputPath, {});
+						const customDir = path.join(
+							outputPath,
+							"this/dir/should/be/removed"
+						);
+						const ignoredDir = path.join(
+							outputPath,
+							"this/is/ignored/dir/that/should/not/be/removed"
+						);
+						fs.mkdirSync(customDir, { recursive: true });
+						fs.writeFileSync(path.join(customDir, "file.ext"), "");
+						fs.mkdirSync(ignoredDir, { recursive: true });
+						fs.writeFileSync(path.join(ignoredDir, "file.ext"), "");
+						once = false;
+					}
 					assets["this/dir/should/not/be/removed/file.ext"] = new RawSource("");
 				});
 			});