From 8f453834e8af78efea2a5de943ca8ab85db9911d Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 6 Jan 2022 17:26:56 -0500 Subject: [PATCH] Fix unsafe reuse of broccoli trees in OneShot The OneShot transform consumes a broccoli tree a single time, detaching us permanently from future updates to that tree, which avoids needing to revalidate all the third-party v1-to-v2 addon conversion during rebuilds. Since OneShot fully consumes a broccoli tree in a separate broccoli pipeline, it's not safe to consume that same tree again in a different pipeline. But an addon can manipulate cacheKeyForTree such that two distinct copies of an addon emit the same broccoli tree instance as their v2Tree, which we send into OneShot. This fails in a very byzantine way. It's the source of the problems people have been having with test-waiters (https://github.com/embroider-build/embroider/issues/1056) because that addon aggressively tries to deduplicate itself. The fix is to not rerun OneShot if we encounter an identical tree a second time. --- packages/compat/src/build-compat-addon.ts | 2 +- packages/compat/src/one-shot.ts | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/compat/src/build-compat-addon.ts b/packages/compat/src/build-compat-addon.ts index 7305ecaf6..0a623935e 100644 --- a/packages/compat/src/build-compat-addon.ts +++ b/packages/compat/src/build-compat-addon.ts @@ -11,7 +11,7 @@ import EmptyPackageTree from './empty-package-tree'; export default function cachedBuildCompatAddon(originalPackage: Package, v1Cache: V1InstanceCache): Node { let tree = buildCompatAddon(originalPackage, v1Cache); if (!originalPackage.mayRebuild) { - tree = new OneShot(tree, originalPackage.name); + tree = OneShot.create(tree, originalPackage.name); } return tree; } diff --git a/packages/compat/src/one-shot.ts b/packages/compat/src/one-shot.ts index e8ad10fc7..3bc7e178e 100644 --- a/packages/compat/src/one-shot.ts +++ b/packages/compat/src/one-shot.ts @@ -17,12 +17,23 @@ class NerfHeimdallBuilder extends Builder { buildHeimdallTree() {} } +let seen = new WeakMap(); + // Wraps a broccoli tree such that it (and everything it depends on) will only // build a single time. export default class OneShot extends Plugin { private builder: NerfHeimdallBuilder | null; - constructor(originalTree: Node, private addonName: string) { + static create(originalTree: Node, privateAddonName: string) { + let output = seen.get(originalTree); + if (!output) { + output = new this(originalTree, privateAddonName); + seen.set(originalTree, output); + } + return output; + } + + private constructor(originalTree: Node, private addonName: string) { // from broccoli's perspective, we don't depend on any input trees! super([], { annotation: `@embroider/compat: ${addonName}`,