168857 2017-02-24 16:51:58 -0800 WebAssembly: exporting a property with a name that's a number doesn't work 2017-04-25 08:36:13 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 159775 1 jfbastien ysuzuki commit-queue jfbastien keith_miller mark.lam msaboff saam ysuzuki oldest_to_newest 1280929 0 jfbastien 2017-02-24 16:51:58 -0800 The following test in test_Instance.js fails but shouldn't: (function ExportedNumber() { for (let i = 0; i <= 129; ++i) { const name = i.toString(); const builder = (new Builder()) .Type().End() .Function().End() .Export() .Function(name) .End() .Code() .Function(name, { params: [], ret: "i32" }) .I32Const(i) .Return() .End() .End(); const bin = builder.WebAssembly().get(); const module = new WebAssembly.Module(bin); const instance = new WebAssembly.Instance(module); const result = instance.exports[name](); assert.isA(result, "number"); assert.eq(result, i); } })(); If you prefix `name` with another string, everything works. I think the issue has to do with using AbstractModuleRecord, somewhere after: symbolTablePutTouchWatchpointSet(moduleEnvironment, state, exp.field, exportedValue, shouldThrowReadOnlyError, ignoreReadOnlyErrors, putResult); exp.field is being put in properly, but then the ModuleRecord mapping is sad at string such as "0". I'm adding this test, commented out, and leaving a FIXME. The spec tests have a similar failure. 1281314 1 ysuzuki 2017-02-26 23:43:34 -0800 Is that the thing? https://bugs.webkit.org/show_bug.cgi?id=168870 1285639 2 jfbastien 2017-03-09 15:22:58 -0800 (In reply to comment #1) > Is that the thing? https://bugs.webkit.org/show_bug.cgi?id=168870 Yeah WebAssembly doesn't limit the names. Right now they're just length + bytes. For JS we further limit to valid UTF-8, details still in flux. I expect numbers and weird Unicode will remain valid though. Maybe the AbstractModuleThing needs a virtual function for this? 1300942 3 ysuzuki 2017-04-25 02:44:06 -0700 I think r213453 already fixes this issue. I'll enable the test in wasm js-api/test_Instance.js. 1300943 4 308079 ysuzuki 2017-04-25 02:50:53 -0700 Created attachment 308079 Patch 1301012 5 308079 jfbastien 2017-04-25 08:06:48 -0700 Comment on attachment 308079 Patch r=me 1301028 6 308079 commit-queue 2017-04-25 08:36:12 -0700 Comment on attachment 308079 Patch Clearing flags on attachment: 308079 Committed r215733: <http://trac.webkit.org/changeset/215733> 1301029 7 commit-queue 2017-04-25 08:36:13 -0700 All reviewed patches have been landed. Closing bug. 308079 2017-04-25 02:50:53 -0700 2017-04-25 08:36:12 -0700 Patch bug-168857-20170425185052.patch text/plain 1520 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1NzI2CmRpZmYgLS1naXQgYS9KU1Rlc3RzL0NoYW5nZUxv ZyBiL0pTVGVzdHMvQ2hhbmdlTG9nCmluZGV4IDA3OGIxNDYxN2I0OWVhMjFkNzM1MTMyMGJiOTM0 Njg5Y2M1ZjE2OTguLjAxY2NjY2ZjYmRlZTZkYjJiZmY4ZWZhM2IxY2E3ZGRjODgyM2U2MjkgMTAw NjQ0Ci0tLSBhL0pTVGVzdHMvQ2hhbmdlTG9nCisrKyBiL0pTVGVzdHMvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMTUgQEAKKzIwMTctMDQtMjUgIFl1c3VrZSBTdXp1a2kgIDx1dGF0YW5lLnRlYUBnbWFp bC5jb20+CisKKyAgICAgICAgV2ViQXNzZW1ibHk6IGV4cG9ydGluZyBhIHByb3BlcnR5IHdpdGgg YSBuYW1lIHRoYXQncyBhIG51bWJlciBkb2Vzbid0IHdvcmsKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2ODg1NworCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIHIyMTM0NTMgYWxsb3dzIFdhc20gaW5zdGFuY2Vz IHRvIGV4cG9zZSBpbmRleGVkIHByb3BlcnRpZXMuIFdlIGFjdGl2YXRlIHRoZSB0ZXN0cy4KKwor ICAgICAgICAqIHdhc20vanMtYXBpL3Rlc3RfSW5zdGFuY2UuanM6CisgICAgICAgIChFeHBvcnRl ZE51bWJlcik6CisKIDIwMTctMDQtMjQgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxl LmNvbT4KIAogICAgICAgICB0ZXN0MjYyOiB0ZXN0MjYyL3Rlc3QvbGFuZ3VhZ2UvZXhwcmVzc2lv bnMvb2JqZWN0L21ldGhvZC1kZWZpbml0aW9uL2Vhcmx5LWVycm9ycy1vYmplY3QtbWV0aG9kLWR1 cGxpY2F0ZS1wYXJhbWV0ZXJzLmpzCmRpZmYgLS1naXQgYS9KU1Rlc3RzL3dhc20vanMtYXBpL3Rl c3RfSW5zdGFuY2UuanMgYi9KU1Rlc3RzL3dhc20vanMtYXBpL3Rlc3RfSW5zdGFuY2UuanMKaW5k ZXggYzUwNmNlOWNlZmRmZDBlZjA3ZTRkZDU3YzljZWYxNTI5NWNjNDY3Ny4uOGQ2M2JlODI4NTI1 NTlhYTY4YTNjMWFhNTY3MTIxNjlmY2E2NmVlZSAxMDA2NDQKLS0tIGEvSlNUZXN0cy93YXNtL2pz LWFwaS90ZXN0X0luc3RhbmNlLmpzCisrKyBiL0pTVGVzdHMvd2FzbS9qcy1hcGkvdGVzdF9JbnN0 YW5jZS5qcwpAQCAtMzAsOCArMzAsNiBAQAogICAgIGFzc2VydC5lcShyZXN1bHQsIDQyKTsKIH0p KCk7CiAKLS8vIEZJWE1FIGV4cG9ydGluZyBhIHByb3BlcnR5IHdpdGggYSBuYW1lIHRoYXQncyBh IG51bWJlciBkb2Vzbid0IHdvcmsgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2ODg1NwotLyoKIChmdW5jdGlvbiBFeHBvcnRlZE51bWJlcigpIHsKICAgICBmb3IgKGxl dCBpID0gMDsgaSA8PSAxMjk7ICsraSkgewogICAgICAgICBjb25zdCBuYW1lID0gaS50b1N0cmlu ZygpOwpAQCAtNTUsNyArNTMsNiBAQAogICAgICAgICBhc3NlcnQuZXEocmVzdWx0LCBpKTsKICAg ICB9CiB9KSgpOwotKi8KIAogY29uc3Qgd2FzbU1vZHVsZVdoaWNoSW1wb3J0SlMgPSAoKSA9PiB7 CiAgICAgY29uc3QgYnVpbGRlciA9IChuZXcgQnVpbGRlcigpKQo=