Windows _getcwd returns the path truncated by _MAX_PATH. But _MAX_PATH is small (260). So when retrieving an absolute path to the cwd for ES6 local FS Module Loader, the retrieved path is easily truncated.
We should open the file with the long UNC file path in Windows. I think using the inode name as the unique module key is not good. By using the inode, we cannot resolve the relative path based on the path of the currently loaded module. That means, (1) load the A/A.js module, (2) load the "./B.js" from A/A.js module, (3) at that time, we would like to load A/B.js So, I think using the absolute path as the module key is better. When landing the Module loading system in WebCore, we will use the URL as the module key.
Created attachment 260820 [details] Patch
Created attachment 260825 [details] Patch
Created attachment 260827 [details] Patch
Comment on attachment 260827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260827&action=review Hooray! It works on Windows! Looks good except a few comments. > Source/JavaScriptCore/jsc.cpp:788 > + for (unsigned i = 0; i < lengthNotIncludingNull; ++i) > + unicodeBuffer[i] = static_cast<UChar>(buffer[i]); This loop doesn't do anything. Just use the first String constructor on the non-null-terminated buffer. > Source/JavaScriptCore/jsc.cpp:874 > + size_t bufferCapacity = 1024; I don't think this is a good way to do this. It's complicated and uses lots of allocation and extra memory. I prefer something like this: fseek(file, 0, SEEK_END); size_t bufferCapacity = ftell(f); fseek(file, 0, SEEK_SET); Then we can do one buffer resize and one fread. > Source/JavaScriptCore/jsc.cpp:886 > + buffer[bufferSize] = '\0'; I don't think it's good to null-terminate the contents of a file. What if the file has a '\0' in it? > Source/JavaScriptCore/jsc.cpp:914 > + auto utf16Vector = longUNCPathName.charactersWithNullTermination(); charactersWithNullTermination isn't used anywhere else in all of WebKit. Let's remove it. Use characters16 instead and null terminate it yourself.
Created attachment 260872 [details] Patch
Comment on attachment 260827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260827&action=review Thanks! >> Source/JavaScriptCore/jsc.cpp:788 >> + unicodeBuffer[i] = static_cast<UChar>(buffer[i]); > > This loop doesn't do anything. Just use the first String constructor on the non-null-terminated buffer. OK, I'll just use String constructor. >> Source/JavaScriptCore/jsc.cpp:874 >> + size_t bufferCapacity = 1024; > > I don't think this is a good way to do this. It's complicated and uses lots of allocation and extra memory. I prefer something like this: > fseek(file, 0, SEEK_END); > size_t bufferCapacity = ftell(f); > fseek(file, 0, SEEK_SET); > > Then we can do one buffer resize and one fread. Simplifying the fillBufferWithContentsOfFile is reasonable. I've changed the implementation. >> Source/JavaScriptCore/jsc.cpp:886 >> + buffer[bufferSize] = '\0'; > > I don't think it's good to null-terminate the contents of a file. What if the file has a '\0' in it? Make sense. I've changed this not to fill with null-termination. And instead, use Vector<char> directly in the caller sides. >> Source/JavaScriptCore/jsc.cpp:914 >> + auto utf16Vector = longUNCPathName.charactersWithNullTermination(); > > charactersWithNullTermination isn't used anywhere else in all of WebKit. Let's remove it. Use characters16 instead and null terminate it yourself. OK, dropped this function from WTFString and use it here as `extractUTF16Characters`.
Created attachment 260901 [details] Patch
Comment on attachment 260901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260901&action=review Thanks to Alex, we fixed the issue related to substring and ensured that the tests pass in Windows. > Source/JavaScriptCore/jsc.cpp:765 > + directoryName.queryName = absolutePathToFile.substring(queryStartPosition, queryLength); Thanks to Alex, we've found one of the issue in the original patch. I misunderstood that String#substring takes the range (substring(start, end)), but it takes the start offset and the length (substring(start, length)).
Comment on attachment 260901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260901&action=review > Source/WTF/wtf/text/WTFString.cpp:-431 > -Vector<UChar> String::charactersWithNullTermination() const Oops, it seems that charactersWithNullTermination is used in WebCore and WebKit code. So, i'll revert this drop.
Created attachment 260905 [details] Patch
Comment on attachment 260905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260905&action=review > Source/JavaScriptCore/jsc.cpp:910 > + auto utf16Vector = longUNCPathName.charactersWithNullTermination(); `charactersWithNullTermination` is used in the WebCore / WebKit side. So in the meantime, we keep this.
Comment on attachment 260905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260905&action=review > Source/WTF/ChangeLog:9 > + (WTF::String::charactersWithNullTermination): Deleted. get rid of this WTF/ChangeLog change. > Source/JavaScriptCore/jsc.cpp:884 > + if (buffer[0] == '#' && buffer[1] == '!') > + buffer[0] = buffer[1] = '/'; What is this? >> Source/JavaScriptCore/jsc.cpp:910 >> + auto utf16Vector = longUNCPathName.charactersWithNullTermination(); > > `charactersWithNullTermination` is used in the WebCore / WebKit side. So in the meantime, we keep this. Sure enough. It's just not used on mac, but this is windows anyway.
Comment on attachment 260905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260905&action=review Thank you for your review. I'll tweak the shebang related thing and land it. >> Source/WTF/ChangeLog:9 >> + (WTF::String::charactersWithNullTermination): Deleted. > > get rid of this WTF/ChangeLog change. Oops, fixed. >> Source/JavaScriptCore/jsc.cpp:884 >> + buffer[0] = buffer[1] = '/'; > > What is this? This was originally introduced to handle the shebang "#!" in JSC shell. (L1866 is the original code) "#!/..." is not valid JS syntax. So it converts shebang to the JS comment. #!/bin/jsc => ///bin/jsc Separating this functionality from the fillBufferWithContentsOfFile is cleaner. I'll split this from fillBufferWithContentsOfFile. > Source/JavaScriptCore/jsc.cpp:-1866 > - buffer[0] = buffer[1] = '/'; Here is the original shebang handling code.
Committed r189583: <http://trac.webkit.org/changeset/189583>
JSC tests fails. ** The following JSC stress test failures have been introduced: jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout-no-llint I guess this is caused due to "rb" open.
(In reply to comment #16) > JSC tests fails. > > ** The following JSC stress test failures have been introduced: > jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js. > layout > jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js. > layout-dfg-eager-no-cjit > jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js. > layout-no-cjit > jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js. > layout-no-llint > jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout > jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout- > dfg-eager-no-cjit > jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout- > no-cjit > jsc-layout-tests.yaml/js/script-tests/parser-high-byte-character.js.layout- > no-llint > > I guess this is caused due to "rb" open. Ah, no. still Windows still uses fopen(..., "r") for the non module files.
I have a small fix. So after ensuring the JSC tests pass in OS X with the change, I'll land it.
Committed r189591: <http://trac.webkit.org/changeset/189591>
Committed r190022: <http://trac.webkit.org/changeset/190022>