Bug 168857 - WebAssembly: exporting a property with a name that's a number doesn't work
Summary: WebAssembly: exporting a property with a name that's a number doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 159775
  Show dependency treegraph
 
Reported: 2017-02-24 16:51 PST by JF Bastien
Modified: 2017-04-25 08:36 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2017-04-25 02:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-02-24 16:51:58 PST
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.
Comment 1 Yusuke Suzuki 2017-02-26 23:43:34 PST
Is that the thing? https://bugs.webkit.org/show_bug.cgi?id=168870
Comment 2 JF Bastien 2017-03-09 15:22:58 PST
(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?
Comment 3 Yusuke Suzuki 2017-04-25 02:44:06 PDT
I think r213453 already fixes this issue.
I'll enable the test in wasm js-api/test_Instance.js.
Comment 4 Yusuke Suzuki 2017-04-25 02:50:53 PDT
Created attachment 308079 [details]
Patch
Comment 5 JF Bastien 2017-04-25 08:06:48 PDT
Comment on attachment 308079 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2017-04-25 08:36:12 PDT
Comment on attachment 308079 [details]
Patch

Clearing flags on attachment: 308079

Committed r215733: <http://trac.webkit.org/changeset/215733>
Comment 7 WebKit Commit Bot 2017-04-25 08:36:13 PDT
All reviewed patches have been landed.  Closing bug.