Bug 148917 - Consider long module path name case in Windows
Summary: Consider long module path name case in Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-06 13:55 PDT by Yusuke Suzuki
Modified: 2015-09-19 18:09 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.21 KB, patch)
2015-09-08 18:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.30 KB, patch)
2015-09-08 19:20 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2015-09-08 20:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.39 KB, patch)
2015-09-09 13:58 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.15 KB, patch)
2015-09-09 18:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.92 KB, patch)
2015-09-09 19:25 PDT, Yusuke Suzuki
achristensen: review+
achristensen: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-09-06 13:55:12 PDT
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.
Comment 1 Yusuke Suzuki 2015-09-06 20:37:48 PDT
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.
Comment 2 Yusuke Suzuki 2015-09-08 18:08:29 PDT
Created attachment 260820 [details]
Patch
Comment 3 Yusuke Suzuki 2015-09-08 19:20:10 PDT
Created attachment 260825 [details]
Patch
Comment 4 Yusuke Suzuki 2015-09-08 20:43:46 PDT
Created attachment 260827 [details]
Patch
Comment 5 Alex Christensen 2015-09-09 09:54:36 PDT
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.
Comment 6 Yusuke Suzuki 2015-09-09 13:58:43 PDT
Created attachment 260872 [details]
Patch
Comment 7 Yusuke Suzuki 2015-09-09 13:59:03 PDT
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`.
Comment 8 Yusuke Suzuki 2015-09-09 18:36:26 PDT
Created attachment 260901 [details]
Patch
Comment 9 Yusuke Suzuki 2015-09-09 18:40:58 PDT
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 10 Yusuke Suzuki 2015-09-09 19:17:07 PDT
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.
Comment 11 Yusuke Suzuki 2015-09-09 19:25:50 PDT
Created attachment 260905 [details]
Patch
Comment 12 Yusuke Suzuki 2015-09-10 11:09:40 PDT
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 13 Alex Christensen 2015-09-10 11:23:31 PDT
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 14 Yusuke Suzuki 2015-09-10 11:51:44 PDT
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.
Comment 15 Yusuke Suzuki 2015-09-10 12:19:24 PDT
Committed r189583: <http://trac.webkit.org/changeset/189583>
Comment 16 Yusuke Suzuki 2015-09-10 13:25:36 PDT
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.
Comment 17 Yusuke Suzuki 2015-09-10 13:33:07 PDT
(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.
Comment 18 Yusuke Suzuki 2015-09-10 14:11:19 PDT
I have a small fix. So after ensuring the JSC tests pass in OS X with the change, I'll land it.
Comment 19 Yusuke Suzuki 2015-09-10 14:25:05 PDT
Committed r189591: <http://trac.webkit.org/changeset/189591>
Comment 20 Yusuke Suzuki 2015-09-19 18:09:31 PDT
Committed r190022: <http://trac.webkit.org/changeset/190022>