Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async function: silent swallowing of the "override mistake" #322

Closed
michaelfig opened this issue Jan 27, 2020 · 7 comments
Closed

async function: silent swallowing of the "override mistake" #322

michaelfig opened this issue Jan 27, 2020 · 7 comments
Labels
fixed - please verify Issue has been fixed. Please verify and close.

Comments

@michaelfig
Copy link

Hi,

The "override mistake" is an EcmaScript spec decision that an own property cannot override a frozen property in its prototype chain. It's a mistake according to @erights because it's a specification that prevents a reasonable use case.

However, given that decision, I would expect the following module code to throw a TypeError:

'use strict'; // for Node's sake; should be superfluous when used as a module
const out = typeof trace === 'undefined' ? console.log : trace;

try {
  const r = (() => {
    function Foo() { }
    Foo.prototype.froze = () => 'is frozen'
    Object.freeze(Foo.prototype)
    const f = new Foo;
    f.froze = () => 'should never happen due to override mistake';
    return f.froze();
  })();
  out(`unexpected sync success: ${r}\n`);
} catch (e) {
  out(`expected sync failure ${e}\n`);
}

(async () => {
  function Foo() { }
  Foo.prototype.froze = () => 'froze'
  Object.freeze(Foo.prototype)
  const f = new Foo;
  f.froze = () => 'nofroze';
  return f.froze();
})().then(
  r => out(`unexpected async success: ${r}\n`),
  r => out(`expected async failure: ${r}\n`)
);

When running as in import from an XS Compartment, I get output:

expected sync failure TypeError: ?: set froze: not writable
unexpected async success: froze

I would expect the async case to throw a similar TypeError. At least it partially implements the override mistake. However, I think this silent failure is a nonconformance, as all module code is supposed to be strict.

When I run it with node t.js, I get:

expected sync failure TypeError: Cannot assign to read only property 'froze' of object '#<Foo>'

expected async failure: TypeError: Cannot assign to read only property 'froze' of object '#<Foo>'

Refs Agoric/agoric-sdk#401

@erights
Copy link

erights commented Jan 28, 2020

It's a mistake according to @erights because it's a specification that prevents a reasonable use case.

And that it violates the principle of least surprise. No one who doesn't already know about this expects things to act that way. Even those who do know often find themselves surprised.

And that its actual behavior has not found a single use case for which it is useful

@erights
Copy link

erights commented Jan 28, 2020

Looks like an XS bug to me. Is there a Moddable bug we can link to? Attn @phoddie

@dckc
Copy link
Contributor

dckc commented Jan 28, 2020

er... this is a Moddable bug, no? i.e. an issue raised against moddable xs.

@erights
Copy link

erights commented Jan 28, 2020

Doh! I didn't realize what repository I was reading.

@bterlson
Copy link
Contributor

@erights not that this is exactly the appropriate venue for this discussion, but I seem to recall back in the 2015s that there were some security concerns WRT overriding frozen properties. Is that not the case at all?

@erights
Copy link

erights commented Jan 31, 2020

The only security issue that comes to mind is exactly the override mistake. By making assignment fail here, you deter people from freezing objects when they should. That's exactly what happened, resulting in a world that's vastly less secure than it would have been otherwise.

I doubt we ever examined an option of preventing override of inherited properties, even by defineProperty. But maybe at some point we did. In any case, it is not a possible option.

mkellner pushed a commit that referenced this issue Oct 14, 2020
@phoddie
Copy link
Collaborator

phoddie commented Oct 14, 2020

The body of async functions executed in sloppy mode. Oops...

@phoddie phoddie added the fixed - please verify Issue has been fixed. Please verify and close. label Oct 14, 2020
@phoddie phoddie closed this as completed Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed - please verify Issue has been fixed. Please verify and close.
Projects
None yet
Development

No branches or pull requests

5 participants