Bug 148917 – Consider long module path name case in Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260820 [details]
Patch

[Yusuke Suzuki]

Created attachment 260825 [details]
Patch

[Yusuke Suzuki]

Created attachment 260827 [details]
Patch

[Alex Christensen]

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.

[Yusuke Suzuki]

Created attachment 260872 [details]
Patch

[Yusuke Suzuki]

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`.

[Yusuke Suzuki]

Created attachment 260901 [details]
Patch

[Yusuke Suzuki]

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)).

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260905 [details]
Patch

[Yusuke Suzuki]

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.

[Alex Christensen]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r189583: <http://trac.webkit.org/changeset/189583>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

I have a small fix. So after ensuring the JSC tests pass in OS X with the change, I'll land it.

[Yusuke Suzuki]

Committed r189591: <http://trac.webkit.org/changeset/189591>

[Yusuke Suzuki]

Committed r190022: <http://trac.webkit.org/changeset/190022>