Skip to content

Commit b93bf7d

Browse files
guybedfordaduh95
authored andcommitted
esm: fix source phase identity bug in loadCache eviction
PR-URL: #62415 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
1 parent 12b04d1 commit b93bf7d

4 files changed

Lines changed: 42 additions & 4 deletions

File tree

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,9 @@ class ModuleJobBase {
145145
*/
146146
syncLink(requestType){
147147
// Store itself into the cache first before linking in case there are circular
148-
// references in the linking.
148+
// references in the linking. Track whether we're overwriting an existing entry
149+
// so we know whether to remove the temporary entry in the finally block.
150+
consthadPreviousEntry=this.loader.loadCache.get(this.url,this.type)!==undefined;
149151
this.loader.loadCache.set(this.url,this.type,this);
150152
constmoduleRequests=this.module.getModuleRequests();
151153
// Modules should be aligned with the moduleRequests array in order.
@@ -169,9 +171,14 @@ class ModuleJobBase {
169171
}
170172
this.module.link(modules);
171173
}finally{
172-
// Restore it - if it succeeds, we'll reset in the caller; Otherwise it's
173-
// not cached and if the error is caught, subsequent attempt would still fail.
174-
this.loader.loadCache.delete(this.url,this.type);
174+
if(!hadPreviousEntry){
175+
// Remove the temporary entry. On failure this ensures subsequent attempts
176+
// don't return a broken job. On success the caller
177+
// (#getOrCreateModuleJobAfterResolve) will re-insert under the correct key.
178+
this.loader.loadCache.delete(this.url,this.type);
179+
}
180+
// If there was a previous entry (ensurePhase() path), leave this in cache -
181+
// it is the upgraded job and the caller will not re-insert.
175182
}
176183

177184
returnevaluationDepJobs;
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// Regression test for source phase import identity with mixed eval/source
2+
// phase imports of the same module in one parent.
3+
import'../common/index.mjs';
4+
import{spawnSyncAndAssert}from'../common/child_process.js';
5+
import*asfixturesfrom'../common/fixtures.mjs';
6+
7+
spawnSyncAndAssert(
8+
process.execPath,
9+
['--no-warnings',fixtures.path('es-modules/test-wasm-source-phase-identity.mjs')],
10+
{stdout: '',stderr: '',trim: true}
11+
);
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import*asmod1from'./simple.wasm';
2+
import*asmod2from'./simple.wasm';
3+
importsourcemod3from'./simple.wasm';
4+
importsourcemod4from'./simple.wasm';
5+
6+
export{mod1,mod2,mod3,mod4};
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import{strictEqual}from'node:assert';
2+
3+
// Pre-load simple.wasm at kSourcePhase to prime the loadCache.
4+
constpreloaded=awaitimport.source('./simple.wasm');
5+
strictEqual(preloadedinstanceofWebAssembly.Module,true);
6+
7+
// Import a parent that has both eval-phase and source-phase imports of the
8+
// same wasm file, which triggers ensurePhase(kEvaluationPhase) on the cached
9+
// job and exposes the loadCache eviction bug.
10+
const{ mod1, mod2, mod3, mod4 }=
11+
awaitimport('./test-wasm-source-phase-identity-parent.mjs');
12+
13+
strictEqual(mod1,mod2,'two eval-phase imports of the same wasm must be identical');
14+
strictEqual(mod3,mod4,'two source-phase imports of the same wasm must be identical');

0 commit comments

Comments
 (0)