The spec says: https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymodule-constructor > The spec string values inside Ast.module are decoded as UTF8 as described in Web.md. Referring to: https://github.com/WebAssembly/design/blob/master/Web.md#names > A WebAssembly module imports and exports functions. WebAssembly names functions using arbitrary-length byte sequences. Any 8-bit values are permitted in a WebAssembly name, including the null byte and byte sequences that don't correspond to any Unicode code point regardless of encoding. The most natural Web representation of a mapping of function names to functions is a JS object in which each function is a property. Property names in JS are UTF-16 encoded strings. A WebAssembly module may fail validation on the Web if it imports or exports functions whose names do not transcode cleanly to UTF-16 according to the following conversion algorithm, assuming that the WebAssembly name is in a Uint8Array called array: > > function convertToJSString(array) > { > var string = ""; > for (var i = 0; i < array.length; ++i) > string += String.fromCharCode(array[i]); > return decodeURIComponent(escape(string)); > } > This performs the UTF8 decoding (decodeURIComponent(escape(string))) using a common JS idiom. Transcoding failure is detected by decodeURIComponent, which may throw URIError. If it does, the WebAssembly module will not validate. This validation rule is only mandatory for Web embedding.
This follows up with Instance's: If the list of module.imports is not empty and Type(importObject) is not Object, a TypeError is thrown.
As well as: Let exports be a list of (string, JS value) pairs that is mapped from each external value e in instance.exports as follows: [...]
*** Bug 164039 has been marked as a duplicate of this bug. ***
Created attachment 293714 [details] patch There's still a bunch more to do, but that's a decent enough spot to get a review and maybe commit.
Attachment 293714 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:100: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293723 [details] patch Un-spaghetti some horrible code that had grown very ugly.
Attachment 293723 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:100: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293726 [details] patch Fix Type section parsing, it now matches the builder's generation.
Attachment 293726 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:130: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293726 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=293726&action=review r- on the lack of assert(...). It makes the code very hard to read. > JSTests/ChangeLog:3 > + WebAssembly JS API: implement more sections This should have a link to the bugzilla. > JSTests/ChangeLog:26 > + Reviewed by NOBODY (OOPS!). I don't think it matters but usually people the "Reviewed by ..." above the comments on what the patch does. > JSTests/wasm/Builder.js:51 > + if (!Array.isArray(params)) throw new Error(`expected a parameter array, got "${params}"`); > + for (const p of params) if (!WASM.isValidValueType(p)) throw new Error(`Type parameter ${p} isn't a valid value type`); > + if (typeof(ret) === "undefined") ret = "void"; > + if (Array.isArray(ret)) throw new Error(`Multiple return values aren't supported by WebAssembly yet`); > + if (ret !== "void" && !WASM.isValidValueType(ret)) throw new Error(`Type return ${ret} isn't a valid value type`); These should all have a new line and elsewhere in patch. It's generally against webkit style to have the if and the single statement on the same line. It might be easier to read if you have an assert function. like: assert(ret !== "void" && !WASM.isValidValueType(ret), `Type return ${ret} isn't a valid value type`); an assert function would make it clearer where the interesting control flow is and where error checking is happening. > JSTests/wasm/Builder.js:69 > + types: nit: I hate labels... > JSTests/wasm/Builder.js:73 > + for (let j = 0; j !== t.params.length; ++j) Should have { } > JSTests/wasm/Builder.js:77 > + break types; I think you can just use break here. > JSTests/wasm/Builder.js:117 > + case "undefined": index = field; break; // Assume it's the same as the field (i.e. it's not being renamed). multi-line
Created attachment 293835 [details] patch Address Keith's comments. Lots of asserts now!
Attachment 293835 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:130: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293867 [details] patch Merge patch and fix new code which used an API that I renamed.
Attachment 293867 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:130: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293835 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=293835&action=review r=me with some more comments. Looks like you need to rebase. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:182 > - > + > if (verbose) > dataLogLn("Got function type."); > - > + > uint32_t argumentCount; > if (!parseVarUInt32(argumentCount)) > return false; > - > + > if (verbose) > dataLogLn("argumentCount: ", argumentCount); > - > + undo. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:288 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:372 > + // FIXME Add a comment / link to bugzilla? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:378 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:409 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmParser.h:47 > + bool WARN_UNUSED_RETURN consumeUTF8String(String &, size_t); String& > Source/JavaScriptCore/wasm/WasmParser.h:100 > +ALWAYS_INLINE bool Parser::consumeUTF8String(String &result, size_t stringLength) Should be String& result > Source/JavaScriptCore/wasm/WasmPlan.h:57 > + const Memory* getMemory() const I think this should just be memory(), that's more consistent with the naming we use elsewhere.
Created attachment 293931 [details] patch Addressed all comments.
Comment on attachment 293931 [details] patch Clearing flags on attachment: 293931 Committed r208401: <http://trac.webkit.org/changeset/208401>
All reviewed patches have been landed. Closing bug.
It looks like this patch breaks all the testWasm tests :(. We need to rebaseline those tests until we can migrate them to the new testing infrastructure.