<rdar://problem/29762017>

Created attachment 297568 [details] patch

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

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 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 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?

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 on attachment 297596 [details] patch r=me

Comment on attachment 297596 [details] patch Clearing flags on attachment: 297596 Committed r210073: <http://trac.webkit.org/changeset/210073>

All reviewed patches have been landed. Closing bug.