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!

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.

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.

(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. ¯\_(ツ)_/¯

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.

Adds eslint rules for js/src/shell and enables the #directives eslint-preprocessor for js/src/shell.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d124e88b043931e60c1536133da610947dac68 Try (part 2 to fix eslint errors): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e3751311e59144f670ab1f42468825ad55a7eb

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.

(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+.

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

I had to back this out for spidermonkey bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=80479983&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4daf78f479

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; } ---

Fixed the Windows issue, re-requesting check-in. New Try for Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9b1debdde8e8a6119cb74df2e113e00b6564b3

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

https://hg.mozilla.org/mozilla-central/rev/fb3ac97e6e79 https://hg.mozilla.org/mozilla-central/rev/bf500219a0fc