Bug 166295

Summary: WebAssembly JS API: cleanup & pass VM around to {Compile/Runtime}Error
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 165805    
Bug Blocks: 159775, 166349    
Attachments:
Description Flags
patch
mark.lam: review-, mark.lam: commit-queue-
patch
none
patch none

Description JF Bastien 2016-12-20 15:55:02 PST
As suggested here: https://bugs.webkit.org/show_bug.cgi?id=165805#c3
These should be updated accordingly.
Comment 1 Radar WebKit Bug Importer 2016-12-20 15:55:44 PST
<rdar://problem/29762017>
Comment 2 JF Bastien 2016-12-20 16:43:05 PST
Created attachment 297568 [details]
patch
Comment 3 Mark Lam 2016-12-20 17:20:08 PST
Comment on attachment 297568 [details]
patch

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

I think this needs some fixes.

> JSTests/wasm/function-tests/trap-store-2.js:49
> +        assert.throws(() => foo(i, address), WebAssembly.RuntimeError, "Out of bounds memory access");

This is a difference in behavior.  The original does "foo(6, address);" instead.  Which one is the bug?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1158
> +            JSWebAssemblyRuntimeError* error = JSWebAssemblyRuntimeError::create(exec, vm, globalObject->WebAssemblyRuntimeErrorStructure(), Wasm::errorMessageForExceptionType(type));

You should use VM& instead of VM* because VM should always be valid.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:35
> +JSWebAssemblyCompileError* JSWebAssemblyCompileError::create(ExecState* state, VM* vm, Structure* structure, const String& message)

Ditto.  Use VM&.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:38
> +    instance->m_sourceAppender = defaultSourceAppender;

This is a change in behavior that is not described in your ChangeLog.  Is this intentional?  If intentional, why not do it in finishCreation() where we are supposed to initialize the object?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.h:39
> +    static JSWebAssemblyCompileError* create(ExecState*, VM*, Structure*, const String&);
> +    static JSWebAssemblyCompileError* create(ExecState* state, VM* vm, Structure* structure, JSValue message)

Use VM& instead of VM*.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:35
> +JSWebAssemblyRuntimeError* JSWebAssemblyRuntimeError::create(ExecState* state, VM* vm, Structure* structure, const String& message)

Use VM&.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:38
> +    instance->m_sourceAppender = defaultSourceAppender;

Do this initialization in finishCreation() where we do all initialization.
Comment 4 JF Bastien 2016-12-20 22:29:44 PST
Comment on attachment 297568 [details]
patch

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

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:38
>> +    instance->m_sourceAppender = defaultSourceAppender;
> 
> This is a change in behavior that is not described in your ChangeLog.  Is this intentional?  If intentional, why not do it in finishCreation() where we are supposed to initialize the object?

That's where ErrorInstance does it, so I did the same. Should I fix both?

Agreed on adding to ChangeLog.
Comment 5 JF Bastien 2016-12-20 22:49:20 PST
Created attachment 297581 [details]
patch

Address other comments:
 - Use VM&.
 - Fix copy-paste mistake in a test.
 - Mention behavior change for source appender in ChangeLog.

See comment above for finishCreation(), I haven't changed it for now, let me know which way to go (change ErrorInstance as well, or leave both as-is).
Comment 6 Mark Lam 2016-12-21 09:18:24 PST
Comment on attachment 297568 [details]
patch

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

>>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:38
>>> +    instance->m_sourceAppender = defaultSourceAppender;
>> 
>> This is a change in behavior that is not described in your ChangeLog.  Is this intentional?  If intentional, why not do it in finishCreation() where we are supposed to initialize the object?
> 
> That's where ErrorInstance does it, so I did the same. Should I fix both?
> 
> Agreed on adding to ChangeLog.

You're right.  I still think it's bad form to do initialization work outside of finishCreation().  We should avoid it.  If we want to be pedantic, then I'd say fix ErrorInstance too in a separate patch.
Comment 7 Mark Lam 2016-12-21 09:28:29 PST
Comment on attachment 297581 [details]
patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:38
> +    instance->m_sourceAppender = defaultSourceAppender;

I'll let this slide for now because of ErrorInstance's bad form.  I think we should fix this eventually to do this initialization in finishCreation().  Otherwise, there's no point in having a separate finishCreation() from this create() function.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:38
> +    instance->m_sourceAppender = defaultSourceAppender;

Ditto.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:39
> +    instance->finishCreation(state, vm, message, true);

We're unconditionally passing true for useCurrentFrame here, but the original client of JSWebAssemblyRuntimeError::create() passes false.  Is this intentional?

Side note: as a matter of style, when passing bools like this, it's hard to tell what that bool is for.  So, instead, this is how we express it to make it clear what we're passing true for:
     bool useCurrentFrame = true;
     instance->finishCreation(state, vm, message, useCurrentFrame);

> Source/JavaScriptCore/wasm/js/WebAssemblyRuntimeErrorConstructor.cpp:54
> -    return JSValue::encode(JSWebAssemblyRuntimeError::create(state, structure, message, false));
> +    return JSValue::encode(JSWebAssemblyRuntimeError::create(state, vm, structure, message));

Originally, we're passing false for useCurrentFrame here but in the new JSWebAssemblyRuntimeError::create() we always pass true.  Is this intentional?
Comment 8 JF Bastien 2016-12-21 09:58:53 PST
Created attachment 297596 [details]
patch

(In reply to comment #7)
> Comment on attachment 297581 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297581&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyCompileError.cpp:38
> > +    instance->m_sourceAppender = defaultSourceAppender;
> 
> I'll let this slide for now because of ErrorInstance's bad form.  I think we
> should fix this eventually to do this initialization in finishCreation(). 
> Otherwise, there's no point in having a separate finishCreation() from this
> create() function.

Sounds good, I'll do this in a follow-up.

> > Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:39
> > +    instance->finishCreation(state, vm, message, true);
> 
> We're unconditionally passing true for useCurrentFrame here, but the
> original client of JSWebAssemblyRuntimeError::create() passes false.  Is
> this intentional?

Ah yes, it seems clearer. Added to the ChangeLog.
 
> Side note: as a matter of style, when passing bools like this, it's hard to
> tell what that bool is for.  So, instead, this is how we express it to make
> it clear what we're passing true for:
>      bool useCurrentFrame = true;
>      instance->finishCreation(state, vm, message, useCurrentFrame);

Done.
Comment 9 Mark Lam 2016-12-21 10:05:08 PST
Comment on attachment 297596 [details]
patch

r=me
Comment 10 WebKit Commit Bot 2016-12-21 12:34:47 PST
Comment on attachment 297596 [details]
patch

Clearing flags on attachment: 297596

Committed r210073: <http://trac.webkit.org/changeset/210073>
Comment 11 WebKit Commit Bot 2016-12-21 12:34:51 PST
All reviewed patches have been landed.  Closing bug.