Bug 161662 – Support jsc shell builtin `read`

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

[JF Bastien]

The jsc shell currently supports a `readFile` method which returns a string. SpiderMonkey's js shell and V8's d8 shell both support similar file-to-string functions, as well as a binary-file-to-Uint8Array function. jsc should support a similar binary file method to simplify testing, including testing of WebAssembly blobs.

Emscripten's shell.js (which is also used for some WebAssembly things) has a polyfill [1] for a builtin called `read`. jsc should therefore have a builtin with the same name if we want things to "Just Work".

  [1]: https://github.com/kripken/emscripten/blob/5f0918409a1407dd168f57cfa34b109cd1770a8a/src/shell.js#L138

[JF Bastien]

Created attachment 288075 [details]
patch

[WebKit Commit Bot]

Attachment 288075 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:1567:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 288075 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288075&action=review

Looks good but I have a couple of comments.

> Source/JavaScriptCore/jsc.cpp:1566
> +    auto content = std::make_unique<Storage>();

Why do you need a unique_ptr here? Wouldn't regular Vector work fine.

> Source/JavaScriptCore/jsc.cpp:1567
> +    auto destroyStorage = [] (void* bytes, void*) { delete static_cast<Storage*>(bytes); };

Doesn't bytes point to the Vector's storage rather than the vector itself? I don't think you want to static_cast this. I think you just want to do fastFree();

> Source/JavaScriptCore/jsc.cpp:1575
> +    JSObjectRef result = JSObjectMakeTypedArrayWithBytesNoCopy(toRef(exec), kJSTypedArrayTypeUint8Array, content->data(), content->size(), destroyStorage, nullptr, &exception);

Why are you calling this through the C-API. I guess there's nothing wrong with that but it just seems harder for you. I would think it's easier to just use the internal TypedArray API.

Also, I think you could just do content->releaseBuffer().leakPtr() instead of content->data() then a release later.

[JF Bastien]

Created attachment 288186 [details]
patch

Address Keith's comments.

(In reply to comment #3)
> Comment on attachment 288075 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288075&action=review
> 
> Looks good but I have a couple of comments.
> 
> > Source/JavaScriptCore/jsc.cpp:1566
> > +    auto content = std::make_unique<Storage>();
> 
> Why do you need a unique_ptr here? Wouldn't regular Vector work fine.

See below, not needed anymore.

> > Source/JavaScriptCore/jsc.cpp:1567
> > +    auto destroyStorage = [] (void* bytes, void*) { delete static_cast<Storage*>(bytes); };
> 
> Doesn't bytes point to the Vector's storage rather than the vector itself? I
> don't think you want to static_cast this. I think you just want to do
> fastFree();

Yup, that was a bug.

> > Source/JavaScriptCore/jsc.cpp:1575
> > +    JSObjectRef result = JSObjectMakeTypedArrayWithBytesNoCopy(toRef(exec), kJSTypedArrayTypeUint8Array, content->data(), content->size(), destroyStorage, nullptr, &exception);
> 
> Why are you calling this through the C-API. I guess there's nothing wrong
> with that but it just seems harder for you. I would think it's easier to
> just use the internal TypedArray API.


I'd tried the C++ one but the JSGenericTypedArrayView<Uint8Adaptor>::create function wasn't exported, causing jsc to fail linking. As discussed I'm now exporting that one specialization.

> Also, I think you could just do content->releaseBuffer().leakPtr() instead
> of content->data() then a release later.

Indeed! Though xcode's navigation I was mistakenly looking at bmalloc's Vector.h instead of the WTF one, and found its capabilities underwhelming for what I wanted to do. This is much better, and removes the need for unique_ptr for auto-lifetime-management-with-release-of-ownership.

[WebKit Commit Bot]

Attachment 288186 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSTypedArrays.cpp:54:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jsc.cpp:1572:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 288187 [details]
patch

Fix format. Ignore lambda format comment.

[WebKit Commit Bot]

Attachment 288187 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:1572:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 288187 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288187&action=review

Other than my one issue it looks good.

> Source/JavaScriptCore/runtime/JSTypedArrays.cpp:54
> +JSUint8Array* createUint8TypedArray(ExecState* exec, Structure* structure, PassRefPtr<ArrayBuffer> passedBuffer, unsigned byteOffset, unsigned length)

The new hotness in WebKit is to use RefPtr<ArrayBuffer>&&. PassRefPtr is a legacy API from before r-value references.

[JF Bastien]

Created attachment 288410 [details]
patch

Address Keith's comment about rvalue ref, and update createUint8TypedArray to forward accordingly.

Rebase from master, changing exception handling to match Mark's change in https://bugs.webkit.org/show_bug.cgi?id=161498

[Mark Lam]

Comment on attachment 288410 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288410&action=review

> Source/JavaScriptCore/jsc.cpp:1566
> +        if (UNLIKELY(scope.exception()) || type != "binary")
> +            return JSValue::encode(throwException(exec, scope, createError(exec, ASCIILiteral("Expected 'binary' as second argument."))));

Throwing an exception while one already exists will result in an assertion failure.  Maybe you should express this as:
    if (UNLIKELY(scope.exception))
        return JSValue();
    if (type != binary)
        return JSValue::encode(throw...);

Or did you deliberately want to overwrite the original exception?  If so, you should declare a CatchScope after the ThrowScope, and do a catchScope.clearException() before doing the throw.

> Source/JavaScriptCore/jsc.cpp:1581
> +    if (UNLIKELY(scope.exception()))
> +        return JSValue::encode(throwException(exec, scope, createError(exec, ASCIILiteral("Failed to allocate ArrayBuffer."))));

Ditto.

[Keith Miller]

Comment on attachment 288410 [details]
patch

r=me.

[Keith Miller]

Comment on attachment 288410 [details]
patch

I'm rescinding my r+ in light of Mark's comments.

[JF Bastien]

Created attachment 288425 [details]
patch

Fixed exceptions. I created a new throwVMError convenience function, which I'll add uses to in a follow-up patch.

[Keith Miller]

Comment on attachment 288425 [details]
patch

r=me if you fix the windows build.

[JF Bastien]

Created attachment 288634 [details]
patch

Fix Windows build. The function signatures didn't match because of the __cdecl calling convention. A lambda should re-wire this properly.


C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\jsc.cpp(1581): error C2664: 'WTF::Ref<JSC::ArrayBuffer> JSC::ArrayBuffer::createFromBytes(const void *,unsigned int,JSC::ArrayBufferDestructorFunction &&)': cannot convert argument 3 from 'void (__cdecl *)(void *)' to 'JSC::ArrayBufferDestructorFunction &&' [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\shell\jscLib.vcxproj]
  C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\jsc.cpp(1581): note: You cannot bind an lvalue to an rvalue reference
C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\jsc.cpp(1581): error C2660: 'JSC::createUint8TypedArray': function does not take 4 arguments [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\shell\jscLib.vcxproj]

[WebKit Commit Bot]

Comment on attachment 288634 [details]
patch

Rejecting attachment 288634 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 288634, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2061969

[JF Bastien]

Created attachment 288637 [details]
patch

Fix ChangeLogs. OOPS!

[WebKit Commit Bot]

Comment on attachment 288637 [details]
patch

Rejecting attachment 288637 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[JF Bastien]

Created attachment 288727 [details]
patch

Same patch, without my silly r+. Just cq+.

[WebKit Commit Bot]

Comment on attachment 288727 [details]
patch

Clearing flags on attachment: 288727

Committed r205880: <http://trac.webkit.org/changeset/205880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.