Closed Bug 1340110 Opened 4 years ago Closed 4 years ago

Shell module loader can't load file from relative path if --module-load-path is present

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: jonco)

Details

Attachments

(1 file)

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
---
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)
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+
(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
https://hg.mozilla.org/mozilla-central/rev/942c217ca90d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.