[EH] Check if Exception.stack is writable

[aheejin]

Apparently WebAssembly.Exception.stack has been readonly all along and I wasn't aware. But I wonder why this has to be readonly, and currently writing to this property works in Chrome and Node and fails in Firefox. I'll ask in the EH repo whether this should be readonly, but in the meantime, we check if this is writable before writing to it.

Temporarily fixes #21044.

Not sure how to test this... Do we run Firefox tests?

[kripken]

Yes, we run all browser tests in firefox by default, so just adding a test to test/test_browser.py will test it in firefox in test-browser-firefox.

[sbc100]

Combine this with the above into a single if ?

Did you verify that this is indeed readonly on FF for you locally?

[aheejin]

Combined within a single if. No I haven't verified that locally because I don't have FF installed... I'll add a browser test so it will be confirmed by the CI.

[sbc100]

I would recommend confirming the issue locally, just to be sure. Is there some reason you don't want to install FF?

[aheejin]

Added a browser test. I don't check the text of stack traces there because we check (the simplified version of) it in test_other.py with Node; this only checks if it runs without an error.

[sbc100]

Not a big deal but I wonder if this test could run the spidermonkey command line vm (and be part of test_core.py)?

Looking at our circleci config it looks like we already run test_demangle_stacks_symbol_map under spidermonkey.

If you want to try doing this locally you can use jsvu to install spidermonkey and do something like this your .emscripten:

SPIDERMONKEY_ENGINE = [os.path.expanduser('~/.jsvu/bin/spidermonkey')]           
JS_ENGINES = [SPIDERMONKEY_ENGINE]

[aheejin]

OK this gets tricky... Yeah I installed spidermonkey and moved the test to test_core.py, with which I added a few functions and decorators for spidermonkey.

One funny thing is spidermonkey doesn't even seem to return anything with Object.getOwnPropertyDescriptor(e, 'stack');, so it is not possible to check if writable is set. So I had to split it again into two ifs:

    if (e.stack) {
      var pd = Object.getOwnPropertyDescriptor(e, 'stack');
      if (pd && pd.writable) {
        var arr = e.stack.split('\n');
        arr.splice(1,1);
        e.stack = arr.join('\n');
      }
    }

But the funny thing is, my spidermonkey doesn't seem to print

TypeError: setting getter-only property "stack"

as in #21044. It just ignores whatever I set it to, but doesn't error out:

js> var tag = new WebAssembly.Tag({parameters: ["i32"]});
js> var ex = new WebAssembly.Exception(tag, [42], {traceStack: true});
js> ex.stack
"@typein:2:10\n"
js> ex.stack = "abc";
"abc"
js> ex.stack
"@typein:2:10\n"

My spidermonkey version is

aheejin@aheejin:~/emscripten$ spidermonkey --version
JavaScript-C122.0

So, yeah, I updated the test but it is not really testing what I want to test, because the way I can think of to test this is to check if the error string contains setting getter-only property "stack" (because, due to exception, the test fails anyway), and now my spidermonkey doesn't emit that string. Not sure if that's due to version differences, or due to the difference between the browser and the shell..?

[bakkot]

spidermonkey doesn't even seem to return anything with Object.getOwnPropertyDescriptor(e, 'stack')

That's because, like all WebIDL attributes, it's a prototype-placed accessor, not an own property. WebIDL readonly attributes are implemented as prototype-placed getters without corresponding setters. Chrome is in violation of the spec in using an own property instead.

As such the way to check is something like

function propertyWritable(obj, prop) {
  let desc = Object.getOwnPropertyDescriptor(obj, prop);
  if (!desc) {
    let proto = Object.getPrototypeOf(obj);
    return proto == null || propertyWritable(proto, prop);
  }
  return "writable" in desc
    ? desc.writable
    : typeof desc.set === "function";
}

propertyWritable(new WebAssembly.Exception(new WebAssembly.Tag({parameters: ["i32"]}), [42], {traceStack: true}), 'stack'); // true in chrome, false in FF

[sbc100]

If setting .stack is not spec compliant then we should probably just not do it... even though its currently possible on chrome.

[bakkot]

Having stack be writable isn't compliant, but if you do want to set the property, you could do Object.defineProperty(e, 'stack', { value: whatever, configurable: true, writable: true }) and that would work even in browsers where .stack is non-writable. defineProperty works even for readonly properties.

(No comment on whether you should do this, just pointing out options.)

[aheejin]

Thanks for the info! But yeah, after all, I think it'd be better not to do this given that this is not spec compliant. Will close this in favor of #21065.