Repository: vercel/next.js
Pull request: fix: amphtml-validator WASM errors (for real)

Rejoice! The day has come. Hopefully, we'll never see `AssertionError: Assertion failed: WebAssembly is uninitialized` ever again. I dug into amphtml-validator's code, and discovered that they have a race condition when initializing. This PR works around it.

The root problem is that `AmpValidator.getInstance` is buggy when multiple calls to it occur in parallel.
See, the point of `AmpValidator.getInstance(validatorPath)` is to re-use instances across multiple calls. It attempts to do that by stashing created instances in `instanceByValidatorJs`.
However, when creating a fresh instance, it is added to `instanceByValidatorJs` **before `instance.init()` is finished**:
https://github.com/ampproject/amphtml/blob/0c8eaba73ca8f5c462a642fa91901a29e6304f6e/validator/js/nodejs/index.js#L309-L313

which means that, if a concurrent call to `AmpValidator.getInstance(validatorPath)` happens before the original `instance.init()` is done, then we'll get back an uninitialized or partially initialized instance:
https://github.com/ampproject/amphtml/blob/0c8eaba73ca8f5c462a642fa91901a29e6304f6e/validator/js/nodejs/index.js#L292-L294

And, since `instance.init()` is the part that handles loading the webassembly code, this race results in the dreaded `AssertionError: Assertion failed: WebAssembly is uninitialized` error when calling methods on the not-yet-initialized validator.

As a workaround, we properly dedupe instance creation on our own. I'll submit a fix to the original repo as well, but this should solve the problem for us.

Changed files:
filename: packages/next/src/export/helpers/get-amp-html-validator.ts
patch: @@ -0,0 +1,40 @@
+import AmpHtmlValidator, {
+  type Validator,
+} from 'next/dist/compiled/amphtml-validator'
+
+const instancePromises = new Map<string | undefined, Promise<Validator>>()
+
+/**
+ * `AmpHtmlValidator.getInstance` is buggy when multiple calls to getInstance occur in parallel.
+ * This wrapper is a workaround for that.
+ *
+ * `AmpHtmlValidator.getInstance(validatorPath)` attempts to re-use the instances across multiple calls
+ * by stashing existing instances in `instanceByValidatorJs`.
+ * However, when creating a fresh instance, it is added to `instanceByValidatorJs` before `instance.init()` is finished:
+ * https://github.com/ampproject/amphtml/blob/0c8eaba73ca8f5c462a642fa91901a29e6304f6e/validator/js/nodejs/index.js#L309-L313
+ *
+ * which means that, if a concurrent call to `AmpHtmlValidator.getInstance(validatorPath)` happens before the original `init()` is done,
+ * then we'll get back an uninitialized or partially initialized instance:
+ * https://github.com/ampproject/amphtml/blob/0c8eaba73ca8f5c462a642fa91901a29e6304f6e/validator/js/nodejs/index.js#L292-L294
+ *
+ * And, since `instance.init()` is the part that handles loading the webassembly code, this race results in
+ * the dreaded "AssertionError: Assertion failed: WebAssembly is uninitialized" error.
+ * As a workaround, we properly dedupe instance creation on our own.
+ * */
+export function getAmpValidatorInstance(
+  validatorPath: string | undefined
+): Promise<Validator> {
+  let promise = instancePromises.get(validatorPath)
+  if (!promise) {
+    // NOTE: if `validatorPath` is undefined, `AmpHtmlValidator` will load the code from its default URL
+    promise = AmpHtmlValidator.getInstance(validatorPath)
+    instancePromises.set(validatorPath, promise)
+  }
+  return promise
+}
+
+export function getBundledAmpValidatorFilepath() {
+  return require.resolve(
+    'next/dist/compiled/amphtml-validator/validator_wasm.js'
+  )
+}

filename: packages/next/src/export/routes/pages.ts
patch: @@ -21,10 +21,13 @@ import {
   SERVER_PROPS_EXPORT_ERROR,
 } from '../../lib/constants'
 import { isBailoutToCSRError } from '../../shared/lib/lazy-dynamic/bailout-to-csr'
-import AmpHtmlValidator from 'next/dist/compiled/amphtml-validator'
 import { FileType, fileExists } from '../../lib/file-exists'
 import { lazyRenderPagesPage } from '../../server/route-modules/pages/module.render'
 import type { MultiFileWriter } from '../../lib/multi-file-writer'
+import {
+  getAmpValidatorInstance,
+  getBundledAmpValidatorFilepath,
+} from '../helpers/get-amp-html-validator'
 
 /**
  * Renders & exports a page associated with the /pages directory
@@ -59,9 +62,7 @@ export async function exportPagesPage(
   }
 
   if (!ampValidatorPath) {
-    ampValidatorPath = require.resolve(
-      'next/dist/compiled/amphtml-validator/validator_wasm.js'
-    )
+    ampValidatorPath = getBundledAmpValidatorFilepath()
   }
 
   const inAmpMode = isInAmpMode(ampState)
@@ -135,7 +136,7 @@ export async function exportPagesPage(
     ampPageName: string,
     validatorPath: string | undefined
   ) => {
-    const validator = await AmpHtmlValidator.getInstance(validatorPath)
+    const validator = await getAmpValidatorInstance(validatorPath)
     const result = validator.validateString(rawAmpHtml)
     const errors = result.errors.filter((e) => e.severity === 'ERROR')
     const warnings = result.errors.filter((e) => e.severity !== 'ERROR')

filename: packages/next/src/server/dev/next-dev-server.ts
patch: @@ -174,27 +174,24 @@ export default class DevServer extends Server {
     )
     this.renderOpts.ampSkipValidation =
       this.nextConfig.experimental?.amp?.skipValidation ?? false
-    this.renderOpts.ampValidator = (html: string, pathname: string) => {
-      const validatorPath =
-        (this.nextConfig.experimental &&
-          this.nextConfig.experimental.amp &&
-          this.nextConfig.experimental.amp.validator) ||
-        require.resolve(
-          'next/dist/compiled/amphtml-validator/validator_wasm.js'
-        )
+    this.renderOpts.ampValidator = async (html: string, pathname: string) => {
+      const { getAmpValidatorInstance, getBundledAmpValidatorFilepath } =
+        require('../../export/helpers/get-amp-html-validator') as typeof import('../../export/helpers/get-amp-html-validator')
 
-      const AmpHtmlValidator =
-        require('next/dist/compiled/amphtml-validator') as typeof import('next/dist/compiled/amphtml-validator')
-      return AmpHtmlValidator.getInstance(validatorPath).then((validator) => {
-        const result = validator.validateString(html)
-        ampValidation(
-          pathname,
-          result.errors
-            .filter((e) => e.severity === 'ERROR')
-            .filter((e) => this._filterAmpDevelopmentScript(html, e)),
-          result.errors.filter((e) => e.severity !== 'ERROR')
-        )
-      })
+      const validatorPath =
+        this.nextConfig.experimental?.amp?.validator ||
+        getBundledAmpValidatorFilepath()
+
+      const validator = await getAmpValidatorInstance(validatorPath)
+
+      const result = validator.validateString(html)
+      ampValidation(
+        pathname,
+        result.errors
+          .filter((e) => e.severity === 'ERROR')
+          .filter((e) => this._filterAmpDevelopmentScript(html, e)),
+        result.errors.filter((e) => e.severity !== 'ERROR')
+      )
     }
 
     const { pagesDir, appDir } = findPagesDir(this.dir)

filename: packages/next/types/compiled.d.ts
patch: @@ -55,6 +55,10 @@ declare module 'next/dist/compiled/jest-worker' {
 }
 
 declare module 'next/dist/compiled/amphtml-validator' {
+  export type Validator = {
+    validateString(html: string): Promise<any>
+  }
+  export function getInstance(validatorPath: string): Promise<Validator>
   export type ValidationError = any
 }
 

filename: test/lib/amp-test-utils.js
patch: @@ -1,14 +1,9 @@
 /* eslint-env jest */
-import amphtmlValidator from 'next/dist/compiled/amphtml-validator'
+import AmpHtmlValidator from 'next/dist/compiled/amphtml-validator'
 
-// Use the same validator that we use for builds.
-// This avoids trying to load one from the network, which can cause random test flakiness.
-// (duplicated from 'packages/next/src/export/routes/pages.ts')
-const validatorPath = require.resolve(
-  'next/dist/compiled/amphtml-validator/validator_wasm.js'
-)
 export async function validateAMP(/** @type {string} */ html) {
-  const validator = await amphtmlValidator.getInstance(validatorPath)
+  const validatorPath = getBundledAmpValidatorFilepath()
+  const validator = await getAmpValidatorInstance(validatorPath)
   const result = validator.validateString(html)
   if (result.status !== 'PASS') {
     for (let ii = 0; ii < result.errors.length; ii++) {
@@ -23,3 +18,38 @@ export async function validateAMP(/** @type {string} */ html) {
   }
   expect(result.status).toBe('PASS')
 }
+
+/** @typedef {import('next/dist/compiled/amphtml-validator').Validator} Validator */
+
+/** @type {Map<string | undefined, Promise<Validator>>} */
+const instancePromises = new Map()
+
+/**
+ * This is a workaround for issues with concurrent `AmpHtmlValidator.getInstance()` calls,
+ * duplicated from 'packages/next/src/export/helpers/get-amp-html-validator.ts'.
+ * see original code for explanation.
+ *
+ * @returns {Promise<Validator>}
+ * */
+function getAmpValidatorInstance(
+  /** @type {string | undefined} */ validatorPath
+) {
+  let promise = instancePromises.get(validatorPath)
+  if (!promise) {
+    // NOTE: if `validatorPath` is undefined, `AmpHtmlValidator` will load the code from its default URL
+    promise = AmpHtmlValidator.getInstance(validatorPath)
+    instancePromises.set(validatorPath, promise)
+  }
+  return promise
+}
+
+/**
+ * Use the same validator that we use for builds.
+ * This avoids trying to load one from the network, which can cause random test flakiness.
+ * (duplicated from 'packages/next/src/export/helpers/get-amp-html-validator.ts')
+ */
+function getBundledAmpValidatorFilepath() {
+  return require.resolve(
+    'next/dist/compiled/amphtml-validator/validator_wasm.js'
+  )
+}