[webkit-reviews] review granted: [Bug 198398] [WASM-References] Add support for Anyref tables, Table.get and Table.set (for Anyref only). : [Attachment 371303] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198398: [WASM-References] Add support for Anyref tables, Table.get and
Table.set (for Anyref only).
https://bugs.webkit.org/show_bug.cgi?id=198398

Attachment 371303: Patch

https://bugs.webkit.org/attachment.cgi?id=371303&action=review




--- Comment #28 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371303
  --> https://bugs.webkit.org/attachment.cgi?id=371303
Patch

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

r=me with comments.

Can you also try to add a test where we'd crash if we didn't hold the lock in
table.grow? Seems like it could be a pure JS test, where we repeatedly call
grow on new tables in a loop, and we collect continuously.

> Source/JavaScriptCore/wasm/WasmTable.h:60
> +    void setOwner(JSObject* owner) { ASSERT(!m_owner); ASSERT(owner);
m_owner = owner; }

style: let's put each statement on its own line of code.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:304
> +	       RefPtr<Wasm::Table> wasmTable =
Wasm::Table::tryCreate(moduleInformation.tableInformation.initial(),
moduleInformation.tableInformation.maximum(),
moduleInformation.tableInformation.type());

Don't you need to type check the tableImport somewhere here? E.g, if we say
we're importing an Anyfunc table, but it's really an Anyref, we should throw,
right?