Closed Bug 1340146 Opened 7 years ago Closed 7 years ago

Shell module loader doesn't detect self-import from main module

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(2 files, 3 obsolete files)

Test case, module file named "m.js":
---
print("EVALUATE M");

import "./m.js";
---

Start shell with: `mozjs -m m.js`

Expected: "EVALUATE M" printed once
Actual: "EVALUATE M" printed twice
Self-importing is quite common for test262 test cases, so this bug blocks validating test failures from multiple test262 tests!
Attached patch bug1340146.patch (obsolete) — Splinter Review
This patch adds a simple path normalization method to Reflect.Loader to ensure that a module request like "/tmp/./m.js" is handled the same as "/tmp/m.js". The normalized path is only used for the |Reflect.Loader.registry| map and not for the actual file system access, because it doesn't all path formats correctly (e.g. Windows UNC paths). 

To fix the test case from comment #0, I also had to change the module-load-path to an absolute path. This is necessary because the initial modules specified through the "-m" command line option are always resolved to an absolute path. And when we later load the same module through an import statement, we need to make sure that |Reflect.Loader.resolve| returns the same module file path.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8840950 - Flags: review?(jcoppeard)
Comment on attachment 8840950 [details] [diff] [review]
bug1340146.patch

Review of attachment 8840950 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/ModuleLoader.js
@@ +65,5 @@
> +            let i = ReflectApply(StringPrototypeIndexOf, path, [pathsep, lastSep]);
> +            if (i < 0)
> +                i = path.length;
> +            let part = ReflectApply(StringPrototypeSubstring, path, [lastSep, i]);
> +            lastSep = i + 1;

This is all fine, but it might be simpler to split the string on the path separator rather than using indexOf and substring.  Also, you could split on a regexp that matched both forward and backward slashes on Windows, simplifying the code above as well.
Attachment #8840950 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #3)
> This is all fine, but it might be simpler to split the string on the path
> separator rather than using indexOf and substring.  Also, you could split on
> a regexp that matched both forward and backward slashes on Windows,
> simplifying the code above as well.

The various ES6 RegExp hooks make it difficult to call String.prototype.split (and RegExp.prototype[Symbol.split]) in a safe way. And since there's this one test suite (test262!) we now run which does horrible things to built-ins to test how resilient implementations are, I went with the manual way of splitting the path into its components. ¯\_(ツ)_/¯
Attached patch bug1340146.patch (obsolete) — Splinter Review
I've updated the patch to add a comment for why it's unsafe to call certain String.prototype built-in functions in shell/ModuleLoader.js. (And I've also replaced the `js::DuplicateString(buffer, strlen(buffer))` call with just `js::DuplicateString(buffer)`, because the latter is simpler and more commonly used.)

Carrying r+ from jonco.
Attachment #8840950 - Attachment is obsolete: true
Attachment #8841569 - Flags: review+
Attached patch bug1340146-part2-eslint.patch (obsolete) — Splinter Review
Adds eslint rules for js/src/shell and enables the #directives eslint-preprocessor for js/src/shell.
Attachment #8841617 - Flags: review?(evilpies)
Comment on attachment 8841617 [details] [diff] [review]
bug1340146-part2-eslint.patch

Review of attachment 8841617 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/ModuleLoader.js
@@ +65,1 @@
>  #endif

I am not sure if eslint would complain about this, but maybe we could do

const pathsep =
#ifdef XP_WIN
"\\";
#else
"/";
#endif

The macro situation sucks, but I can't think of a better solution that ensures all branches are ok.
Attachment #8841617 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #8)
> ::: js/src/shell/ModuleLoader.js
> @@ +65,1 @@
> >  #endif
> 
> I am not sure if eslint would complain about this, but maybe we could do
> 
> const pathsep =
> #ifdef XP_WIN
> "\\";
> #else
> "/";
> #endif

Yes, that works (https://treeherder.mozilla.org/#/jobs?repo=try&revision=03629887f1fb6f7a23a4850ef9995fdb4f54b3bf). I'll update the patch.

> 
> The macro situation sucks, but I can't think of a better solution that
> ensures all branches are ok.

Well, it's probably not worth it to spend time right now to get a better integration of the #directives into eslint, so I wouldn't worry about it. :-)
Updated part 2 per evilpie's recommendation, carrying r+.
Attachment #8841617 - Attachment is obsolete: true
Attachment #8841657 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c355a0ffcb10
Normalize paths for module registry in shell module loader. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5556b171034e
Process js/src/shell JavaScript files as self-hosted in eslint plugin. r=evilpie
Keywords: checkin-needed
Removes leading white-space before #directives to unbreak Windows builds. Carrying r+ from jonco.


With leading whitespace, the preprocessor adds a "#line" annotation into the generated JavaScript code:
---
        let normalized = ReflectApply(ArrayPrototypeJoin, components, [pathsep]);
        normalized = drive + normalized;
    #line 112 "c:/hg/mozilla-inbound/js/src/shell/ModuleLoader.js"
        return normalized;
    }
---
Attachment #8841569 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8841932 - Flags: review+
Fixed the Windows issue, re-requesting check-in. 

New Try for Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9b1debdde8e8a6119cb74df2e113e00b6564b3
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3ac97e6e79
Part 1: Normalize paths for module registry in shell module loader. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf500219a0fc
Part 2: Process js/src/shell JavaScript files as self-hosted in eslint plugin. r=evilpie
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.