598176 - Object.defineProperty should avoid JS API inside engine, plus a few other cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

Attachment: proposed fix

Summary says it all. Patch is obvious brute force.

/be

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 476943 [details] [diff] [review]
proposed fix

var propdesc = {get: function() {}};
var a = Object.defineProperty({}, "prop", propdesc);
var info = Object.getOwnPropertyDescriptor(a, "prop");

info.get.foo(); // don't crash

assertEq(propdesc.get, info.get);  // quantum entanglement needed

Comment 2

[Brendan Eich [:brendan]]

D'oh, will try to fix. If it's too much work I'll morph to make this only about JS API usage under the hood. Thanks,

/be

Comment 3

[Brendan Eich [:brendan]]

I was way too ambitious. You can have a method barrier on a function-valued property, but an accessor property would need barriers on its getter and setter pointers. Backing off.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: cleanup patch

Just shorter/canonical names (e.g. found), bool not JSBool (at price of one !!), and internal APIs not JS APIs. Also two deadwood method declarations pruned from PropDesc.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: more cleanup, plus jorendorff's test

Comment 6

[Brendan Eich [:brendan]]

I took a second look, it is doable to add more method-barrier magic. It's too much code and not worth it, though, at least not without data from the field.

One ES5 meta-programming design goal was to make the property descriptor in some ways "declarative", to reduce cost and room for mutation. Evaluation of function expressions creates a fresh object per expression eval'ed, without optimizations that are hard to do without aggressive static analysis.

We might get there via bhackett's static analysis. But piling on more dynamic analysis is not called for right now.

/be

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 476983 [details] [diff] [review]
more cleanup, plus jorendorff's test

>+assertEq(false, r1.desc.get === r2.desc.get);

The semantics here are impeccable, but FWIW the error message assertEq generates on failure assumes you've done assertEq(actual, expected):

  js> r1 = r2;
  js> assertEq(false, r1.desc.get === r2.desc.get);
  typein:5: Error: Assertion failed: got false, expected true
  js> assertEq(r1.desc.get === r2.desc.get, false);
  typein:6: Error: Assertion failed: got true, expected false

r=me, no action required.

Comment 8

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/97f175be11e9

/be

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> http://hg.mozilla.org/tracemonkey/rev/97f175be11e9

This triggered orange across platforms in tests/js1_8_5/regress/regress-597945-1.js :

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/tracemonkey_leopard_test-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_8_5/regress/regress-597945-1.js | Unknown file:///Users/cltbld/talos-slave/tracemonkey_leopard_test-jsreftest/build/jsreftest/tests/js1_8_5/regress/regress-597945-1.js:16: arguments is not defined item 1

Comment 10

[Brendan Eich [:brendan]]

Sorry, mq revived a bogus pair of tests/js1_8_5/regress/jstests.list lines:

http://hg.mozilla.org/tracemonkey/rev/9a25714382f4

/be

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/97f175be11e9