Bug 164023

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.