Closed
Bug 1340110
Opened 7 years ago
Closed 7 years ago
Shell module loader can't load file from relative path if --module-load-path is present
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: anba, Assigned: jonco)
Details
Attachments
(1 file)
5.02 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Create a modules directory with an empty module file: `cd /tmp; mkdir modules; touch modules/m.js` 2. Try to load the module: `LANG=C mozjs --module-load-path=/tmp/modules --module modules/m.js` Expected: - No error Actual: - Throws this error: --- shell/ModuleLoader.js:12:16 Error: can't open /tmp/modules/modules/m.js: No such file or directory Stack: fetch@shell/ModuleLoader.js:12:16 loadAndParse@shell/ModuleLoader.js:18:22 Reflect.Loader<@shell/ModuleLoader.js:24:22 --- When using "--module m.js" instead of "--module modules/m.js", the following error is thrown: --- Error: can't open m.js: No such file or directory ---
Assignee | ||
Comment 1•7 years ago
|
||
Currently we try to load all modules via the load path if given. This means that specifying a relative path like this will look for the file relative to the load path and fail as seen. The second case fails for a different reason: the shell expects the --module argument to be relative to the current directory, as it is for scripts, and fails because it can't open the file. I think the best course of action is to make --module work like the existing script arguments and resolve relative to the current directory, with imports only resolving relative to the load path. This is probably the least surprising thing to do. What do you think?
Assignee: nobody → jcoppeard
Attachment #8840023 -
Flags: review?(andrebargull)
Reporter | ||
Comment 2•7 years ago
|
||
Comment on attachment 8840023 [details] [diff] [review] bug1340110-shell-module-loader Review of attachment 8840023 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jon Coppeard (:jonco) from comment #1) > I think the best course of action is to make --module work like the existing > script arguments and resolve relative to the current directory, with imports > only resolving relative to the load path. This is probably the least > surprising thing to do. > > What do you think? Agreed, this sounds like the right thing to do. ::: js/src/shell/ModuleLoader.js @@ +41,5 @@ > + return this.loadAndExecute(path); > + } > + > + ["import"](name, referrer) { > + let path = this.resolve(requestName); requestName -> name ::: js/src/shell/OSObject.cpp @@ +123,5 @@ > + return nullptr; > + > + if (strcmp(scriptFilename.get(), "-e") == 0 || strcmp(scriptFilename.get(), "typein") == 0) > + resolveMode = RootRelative; > + Just moving these lines doesn't seem correct to me, |resolveMode| is never read after this point. I assume you actually wanted to guard the lines with an additional |if (resolveMode == ScriptRelative) { ... }| block. ::: js/src/shell/js.cpp @@ +758,5 @@ > RootedObject loaderObj(cx); > MOZ_ALWAYS_TRUE(GetLoaderObject(cx, &loaderObj)); > > RootedFunction importFun(cx); > + MOZ_ALWAYS_TRUE(GetImportRootMethod(cx, loaderObj, &importFun)); I wonder if we should handle the case when importRoot (or Loader itself) was modified/deleted to avoid an assertion failure: `mozjs -e "Reflect.Loader.importRoot=null" -m /tmp/empty.js`
Attachment #8840023 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to André Bargull from comment #2) > Just moving these lines doesn't seem correct to me, |resolveMode| is never > read after this point. I assume you actually wanted to guard the lines with > an additional |if (resolveMode == ScriptRelative) { ... }| block. Ugh, yes, cheers. > I wonder if we should handle the case when importRoot (or Loader itself) was > modified/deleted to avoid an assertion failure: `mozjs -e > "Reflect.Loader.importRoot=null" -m /tmp/empty.js` Good idea, I'll make this check and report an error instead of asserting.
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/942c217ca90d Make shell resolve root module relative to current directory as happens for non-module scripts r=anba
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/942c217ca90d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•