Normative: Add three missing checks in proxy internal methods #666

claudepache · 2016-08-12T17:38:13Z

Those checks are needed in order to maintain the Invariants of the essential internal methods. Their omission is a bug in the spec. This is a minimal patch for resolving them.

It remains glitches in some weird edge cases (commented below), but they need a little more refactoring.

/cc @erights, @tvcutsem

Fixes #652

bterlson · 2016-08-12T17:39:18Z

Is this in regards to #652?

claudepache · 2016-08-12T17:40:40Z

@bterlson Yes, forgot to mention it.

claudepache · 2016-08-12T17:43:31Z

Testcase against [[GetOwnProperty]]: a property reported as nonwritable, nonconfigurable must not change its value.

var target = Object.seal({x: 2})
var proxy = new Proxy(target, {
    getOwnPropertyDescriptor(o, p) { 
        var desc = Reflect.getOwnPropertyDescriptor(o, p)
        if (desc && 'writable' in desc)
            desc.writable = false
        return desc
    }
})

Object.getOwnPropertyDescriptor(proxy, 'x') // !!! should throw
// { value: 2, configurable: false, writable: false }
Object.defineProperty(proxy, 'x', { value: 3 })
Object.getOwnPropertyDescriptor(proxy, 'x') // { value: 3 }

Testcase against [[DefineProperty]]: a property defined as nonwritable, nonconfigurable must not change its value.

var target = Object.seal({x: 2})
var proxy = new Proxy(target, {
    defineProperty(o, p, desc) {
        delete desc.writable
        return Reflect.defineProperty(o, p, desc)
    }
})

Object.defineProperty(proxy, 'x', { value: 2, configurable: false, writable: false })
// !!! should throw
proxy.x = 3
Object.getOwnPropertyDescriptor(proxy, 'x') // { value: 3 }

Testcase against [[Delete]]: a successfuly deleted property on a nonextensible object must not reappear.

var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, { 
   deleteProperty() { return true } 
})

Object.isExtensible(proxy) // false
delete proxy.x // true !!! should throw
proxy.hasOwnProperty('x') // true

claudepache · 2016-08-12T17:55:40Z

There is an issue (unrelated to invariants of essential methods) in proxy's [[OwnPropertyKeys]] method in case the target object's [[OwnPropertyKeys]] produces duplicate keys, see message on es-discuss. But that is better resolved by #461 (comment) (forbid [[OwnPropertyKeys]] returning duplicate keys).

claudepache · 2016-08-12T18:13:21Z

Finally there are issues with a strong interpretation of the Invariants of the essential internal methods in edge cases, see this thread on es-discuss.

In case we want to resolve that, we need to:

add invariant checks to ordinary [[Get]], [[Set]] and [[HasProperty]] internal methods, because they can run user code that might do weird things;
refactor some invariant checks of proxies (I think, that would essentially result in omitting a few useless calls to the target's internal methods, but that needs to be confirmed).

domenic · 2016-08-12T18:20:30Z

This feels like "needs consensus", as it will cause slowdowns and such...

bterlson · 2016-08-12T18:23:41Z

Definitely needs-consensus. Also I wonder if we should package all of the proxy invariant fixes into a single PR? Removing duplicate keys for example would have some other effects (such as being able to revert 6d0fc45).

erights · 2016-08-12T18:26:16Z

+1 on needs consensus. I am willing to lead but have asked Claude and Tom
to attend by hangout. +1 also on consolidating these issues.

On Fri, Aug 12, 2016 at 11:23 AM, Brian Terlson notifications@github.com
wrote:

Definitely needs-consensus. Also I wonder if we should package all of the
proxy invariant fixes into a single PR? Removing duplicate keys for example
would have some other effects (such as being able to revert 6d0fc45
6d0fc45
).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#666 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAQtzGj9dFqogyrJbz5Ww-Qq0MJQ-HX3ks5qfLowgaJpZM4JjTK5
.

Cheers,
--MarkM

claudepache · 2016-08-12T18:36:02Z

In the present PR, I have purposely included only the three missing checks, because they are real bugs that should not need much discussion and should IMHO be corrected rapidly.

tvcutsem · 2016-08-16T07:23:26Z

I have reviewed Claude’s changes in this PR and they appear to resolve the three outstanding issues as discussed on esdiscuss.

I agree with Claude that it would be better to treat this issue separately from other, partially related discussions. This PR addresses missing proxy invariant checks that I consider to be a bug in the original Proxy spec.

bterlson · 2016-08-16T17:07:07Z

I would be fine pulling in just these additional invariants, but let me wait until tomorrow in case there are objections.

Any volunteers to write tests?

littledan · 2016-08-17T20:26:34Z

cc @verwaest

littledan · 2016-08-17T20:48:55Z

I'm trying to get a review of this from the people who implemented Proxies in V8. Can we hold off on merging this for a little bit until we can hear back from them?

ajklein · 2016-08-17T20:55:51Z

How confident are we that these three additions are sufficient to fix the bug (and don't introduce other problems)? There exist several implementations of the current spec, buggy though it may be, and it's slightly concerning to change the spec in such a way that it matches no implementations.

Given that there's a plan to add more checks here it seems reasonable to ask for more feedback before merging (most naturally by consensus).

erights · 2016-08-17T21:05:34Z

We certainly do not have consensus at the present time. This must not be merged until we do.

…proxy invariant checks in getOwnPropertyDescriptor, defineProperty and deleteProperty

tvcutsem · 2016-08-17T21:35:11Z

FWIW, I added the proposed missing proxy invariant checks to my harmony-reflect shim, and using Claude's test-cases confirmed that all three cases now throw an appropriate TypeError. The changes do not impact the results of any of my other tests, which is good.

Harmony-reflect is obviously not a proper engine implementation, but it does aim to faithfully self-host the Proxy spec in JS.

The changes I had to make follow the exact same pattern of the pre-existing invariant checks. This provides some evidence that these new invariant checks should not be problematic, given the overhead already imposed by pre-existing invariant checks.

claudepache · 2016-09-21T23:50:31Z

How confident are we that these three additions are sufficient to fix the bug (and don't introduce other problems)?

If you are interested in a formal proof, see: https://github.com/claudepache/es-invariants (WIP)

bterlson · 2016-11-29T18:55:18Z

Did tests ever land for this? /cc @leobalter @tcare.

tvcutsem · 2016-12-04T20:45:43Z

After the september TC39 meeting I promised to write up some test262 tests, but I never got around to it. If this is still a blocking issue, I'll see if I can at least contribute Claude's test cases from this thread.

bterlson · 2016-12-05T17:05:21Z

@tvcutsem it is blocking, yes. Let me know if you need help with the porting effort.

raulsebastianmihaila · 2017-02-28T17:36:31Z

Looks like these 3 new invariants are only added in section 9.5. They should also be added in section 6.1.7.3.

Edit: Ignore my comment, these invariants are proxy specific.

tvcutsem · 2017-03-12T12:28:58Z

Contributing test262 tests for these changes has been on my todo list for way too long and other priorities keep on interfering. Part of the reason is I'm not familiar enough with test262 guidelines. Anyone willing to port Claude's test cases above #666 (comment) to test262?

Apologies for slacking off.

littledan · 2018-03-05T10:49:00Z

How is this effort going? Has anyone written tests for this change?

ljharb · 2019-05-03T17:07:53Z

spec.html

@@ -8467,6 +8469,9 @@ <h1>[[GetOwnProperty]] (_P_)</h1>
          <li>
            A property cannot be reported as non-configurable, if it does not exist as an own property of the target object or if it exists as a configurable own property of the target object.
          </li>
+          <li>
+            A property cannot be reported as both non-configurable and non-writable, unless it exists as a non-configurable, non-writable own property of the target object.


This sentence uses “unless it exists as an X own”, while the others in this section use “if it does not exist as an own, or if it exists as an X own” - i like your wording better, so maybe we should update them all to use this form? That’s also fine to do in a followup, but in that case, this should probably follow the form of the others in the interim?

littledan · 2019-05-15T10:07:57Z

I'd like to suggest that we don't land this patch until we have implementation experience (ideally multiple). Would it make sense to include a label for this state, like "awaiting implementation feedback"?

ljharb · 2019-05-15T12:50:42Z

For reference, the tests:

I suppose we could create a label; are there other PRs that would fall into that category?

leobalter · 2019-05-15T15:40:16Z

I suppose we could create a label; are there other PRs that would fall into that category?

@ljharb If it helps, we can also use a feature tag in Test262 to keep track of these tests without listing each one separately.

evilpie · 2019-05-15T16:08:53Z

I think I am going to try this in SpiderMonkey. Firefox actually uses quite a few proxies, so it might be interesting to see what a complete test run uncovers.

https://bugzilla.mozilla.org/show_bug.cgi?id=1551939

ljharb · 2019-05-15T18:43:38Z

@leobalter sure, any way for people reading this PR to quickly see the actual implementation status is great :-)

evilpie · 2019-06-04T09:17:31Z

This change has been in Firefox Nightly for about a week.

erights

Perhaps phrasing should be made more consistent along the lines @ljharb suggests. I'll leave that for others.

As for technical content, all looks good. I approve.

ljharb

Just one final callout - I've reworded these three comments so they're using similar "unless" phrasing. Any objections before this is merged?

ljharb · 2019-06-27T06:19:10Z

spec.html

          </li>
          <li>
-            A property cannot be reported as non-configurable, if it does not exist as an own property of the target object or if it exists as a configurable own property of the target object.
+            A property cannot be reported as both non-configurable and non-writable, unless it exists as a non-configurable, non-writable own property of the target object.


i've reworded this

This phrase is correct and corresponds to the added check.

ljharb · 2019-06-27T06:19:14Z

spec.html

          </li>
          <li>
-            A property cannot be reported as existent, if it does not exist as an own property of the target object and the target object is not extensible.
+            A property cannot be reported as non-configurable, unless it exists as a non-configurable own property of the target object.


i've reworded this

GitHub does not show the reworded phrase in regard to the old one, so it confused me at first. Therefore I confirm that:

The new phrase just before that one (in source text order, L.8988) is logically equivalent to the one shown at left. (L.8986)

This phrase (L.8991) is a correct rewording of the next one (L.8989)

ljharb · 2019-06-27T06:19:17Z

spec.html

@@ -8980,13 +8982,16 @@ <h1>[[GetOwnProperty]] ( _P_ )</h1>
            A property cannot be reported as non-existent, if it exists as a non-configurable own property of the target object.
          </li>
          <li>
-            A property cannot be reported as non-existent, if it exists as an own property of the target object and the target object is not extensible.
+            A property cannot be reported as non-existent, if the target object is not extensible, unless it does not exist as an own property of the target object.


i've reworded this

The rewording is logically correct, but it makes me more difficult to parse: two dependant clauses: “if... unless”; double negation: “unless ... not...”.

@claudepache what about:

Suggested change

A property cannot be reported as non-existent, if the target object is not extensible, unless it does not exist as an own property of the target object.

A property cannot be reported as non-existent, unless either the target object is extensible, or it does not exist as an own property of the target object.

That’s logically correct. But I’m not convinced that the rewording adds clarity. (In any case, this is not a blocker, since it is just an informative note.)

https://bugs.webkit.org/show_bug.cgi?id=198630 Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-07-24 Reviewed by Darin Adler. JSTests: * stress/proxy-delete.js: Assert isExtensible is called in correct order. * test262/expectations.yaml: Mark 6 test cases as passing. Source/JavaScriptCore: Add three missing checks in Proxy internal methods. These checks are necessary to maintain the invariants of the essential internal methods. (tc39/ecma262#666) 1. [[GetOwnProperty]] shouldn't return non-configurable and non-writable descriptor when the target's property is writable. 2. [[Delete]] should return `false` when the target has property and is not extensible. 3. [[DefineOwnProperty]] should return `true` for a non-writable input descriptor when the target's property is non-configurable and writable. Shipping in SpiderMonkey since https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69) Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919 * runtime/ProxyObject.cpp: (JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check. (JSC::ProxyObject::performDelete): Add extensibility check. (JSC::ProxyObject::performDefineOwnProperty): Add writability check. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@247811 268f45cc-cd09-0410-ab3c-d52691b4dbfc

michaelficarra · 2019-11-08T23:21:59Z

spec.html

@@ -8968,6 +8968,8 @@ <h1>[[GetOwnProperty]] ( _P_ )</h1>
        1. If _resultDesc_.[[Configurable]] is *false*, then
          1. If _targetDesc_ is *undefined* or _targetDesc_.[[Configurable]] is *true*, then
            1. Throw a *TypeError* exception.
+          1. If _resultDesc_ has a [[Writable]] field and _resultDesc_.[[Writable]] is *false*, then
+            1. If _targetDesc_.[[Writable]] is *true*, throw a *TypeError* exception.


Why do we have an "if" inside another "if"? Just combine the conditions.

generally its just about making it easier to understand the logic for readers. the outer one contains invariants of the resultDesc and the inner one contains invariants of the targetDesc.

raulsebastianmihaila · 2020-01-05T16:07:13Z

I'm curious why this is still not merged. Is there still something that needs to be done? Thanks.

ljharb · 2020-01-05T17:08:21Z

@raulsebastianmihaila yes; waiting for a reply on #666 (comment) from @claudepache primarily.

littledan · 2020-02-04T10:49:43Z

What's the current implementation status of this change? Is it shipping in Firefox, @evilpie ?

syg · 2020-02-05T00:13:42Z

Judging by the tests in #666 (comment), V8 implements the 3 checks.

ljharb · 2020-02-05T00:25:49Z

Per the test262.report links in #666 (comment), every reported engine but Chakra currently implements this PR.

https://bugs.webkit.org/show_bug.cgi?id=198630 Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-07-24 Reviewed by Darin Adler. JSTests: * stress/proxy-delete.js: Assert isExtensible is called in correct order. * test262/expectations.yaml: Mark 6 test cases as passing. Source/JavaScriptCore: Add three missing checks in Proxy internal methods. These checks are necessary to maintain the invariants of the essential internal methods. (tc39/ecma262#666) 1. [[GetOwnProperty]] shouldn't return non-configurable and non-writable descriptor when the target's property is writable. 2. [[Delete]] should return `false` when the target has property and is not extensible. 3. [[DefineOwnProperty]] should return `true` for a non-writable input descriptor when the target's property is non-configurable and writable. Shipping in SpiderMonkey since https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69) Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919 * runtime/ProxyObject.cpp: (JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check. (JSC::ProxyObject::performDelete): Add extensibility check. (JSC::ProxyObject::performDefineOwnProperty): Add writability check. Canonical link: https://commits.webkit.org/213935@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247811 268f45cc-cd09-0410-ab3c-d52691b4dbfc

bterlson added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Aug 12, 2016

bterlson added the needs consensus This needs committee consensus before it can be eligible to be merged. label Aug 12, 2016

tvcutsem pushed a commit to tvcutsem/harmony-reflect that referenced this pull request Aug 17, 2016

added bugfixes reported in tc39 issue666 <tc39/ecma262#666>, missing …

99ee7f5

…proxy invariant checks in getOwnPropertyDescriptor, defineProperty and deleteProperty

claudepache mentioned this pull request Sep 21, 2016

+ Consolidating Proxy integrity checks tc39/agendas#217

Merged

bterlson removed the needs consensus This needs committee consensus before it can be eligible to be merged. label Nov 29, 2016

raulsebastianmihaila mentioned this pull request Jan 6, 2018

PRs policy #1061

Closed

ljharb approved these changes May 3, 2019

View reviewed changes

ljharb requested review from erights and tvcutsem May 3, 2019 17:09

shvaikalesh mentioned this pull request Jun 7, 2019

Add "proxy-missing-checks" feature tag tc39/test262#2195

Merged

zenparsing approved these changes Jun 26, 2019

View reviewed changes

ljharb self-assigned this Jun 26, 2019

erights approved these changes Jun 26, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

ljharb force-pushed the proxy-missing-checks branch from 92f0555 to eb79f07 Compare June 27, 2019 06:18

ljharb pushed a commit to claudepache/ecma262 that referenced this pull request Jun 27, 2019

Normative: Add three missing checks in proxy internal methods (tc39#666)

eb79f07

ljharb reviewed Jun 27, 2019

View reviewed changes

michaelficarra reviewed Nov 8, 2019

View reviewed changes

Normative: Add three missing checks in proxy internal methods (tc39#666)

b41a832

ljharb force-pushed the proxy-missing-checks branch from eb79f07 to b41a832 Compare January 6, 2020 18:57

ljharb merged commit b41a832 into tc39:master Jan 6, 2020

raulsebastianmihaila mentioned this pull request Mar 15, 2021

Inaccurate definition of essential internal methods #2345

Open

	A property cannot be reported as non-existent, if the target object is not extensible, unless it does not exist as an own property of the target object.
	A property cannot be reported as non-existent, unless either the target object is extensible, or it does not exist as an own property of the target object.

Normative: Add three missing checks in proxy internal methods #666

Normative: Add three missing checks in proxy internal methods #666

Conversation

claudepache commented Aug 12, 2016 • edited Loading

bterlson commented Aug 12, 2016

claudepache commented Aug 12, 2016

claudepache commented Aug 12, 2016

claudepache commented Aug 12, 2016 • edited Loading

claudepache commented Aug 12, 2016 • edited Loading

domenic commented Aug 12, 2016

bterlson commented Aug 12, 2016

erights commented Aug 12, 2016

claudepache commented Aug 12, 2016

tvcutsem commented Aug 16, 2016

bterlson commented Aug 16, 2016

littledan commented Aug 17, 2016

littledan commented Aug 17, 2016

ajklein commented Aug 17, 2016

erights commented Aug 17, 2016

tvcutsem commented Aug 17, 2016

claudepache commented Sep 21, 2016

bterlson commented Nov 29, 2016

tvcutsem commented Dec 4, 2016

bterlson commented Dec 5, 2016

raulsebastianmihaila commented Feb 28, 2017 • edited Loading

tvcutsem commented Mar 12, 2017

littledan commented Mar 5, 2018

Choose a reason for hiding this comment

littledan commented May 15, 2019

ljharb commented May 15, 2019

leobalter commented May 15, 2019

evilpie commented May 15, 2019 • edited Loading

ljharb commented May 15, 2019

evilpie commented Jun 4, 2019

erights left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulsebastianmihaila commented Jan 5, 2020

ljharb commented Jan 5, 2020

littledan commented Feb 4, 2020

syg commented Feb 5, 2020

ljharb commented Feb 5, 2020

claudepache commented Aug 12, 2016 •

edited

Loading

claudepache commented Aug 12, 2016 •

edited

Loading

claudepache commented Aug 12, 2016 •

edited

Loading

raulsebastianmihaila commented Feb 28, 2017 •

edited

Loading

evilpie commented May 15, 2019 •

edited

Loading