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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@claudepache
Copy link
Contributor

claudepache commented Aug 12, 2016

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

This comment has been minimized.

Copy link
Member

bterlson commented Aug 12, 2016

Is this in regards to #652?

@claudepache

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Aug 12, 2016

@bterlson Yes, forgot to mention it.

@claudepache

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Aug 12, 2016

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

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Aug 12, 2016

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

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Aug 12, 2016

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

This comment has been minimized.

Copy link
Member

domenic commented Aug 12, 2016

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

@bterlson

This comment has been minimized.

Copy link
Member

bterlson commented Aug 12, 2016

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

This comment has been minimized.

Copy link

erights commented Aug 12, 2016

+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

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Aug 12, 2016

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

This comment has been minimized.

Copy link

tvcutsem commented Aug 16, 2016

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

This comment has been minimized.

Copy link
Member

bterlson commented Aug 16, 2016

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

This comment has been minimized.

Copy link
Member

littledan commented Aug 17, 2016

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Aug 17, 2016

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

This comment has been minimized.

Copy link
Contributor

ajklein commented Aug 17, 2016

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

This comment has been minimized.

Copy link

erights commented Aug 17, 2016

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

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

added bugfixes reported in tc39 issue666 <tc39/ecma262#666>, missing …
…proxy invariant checks in getOwnPropertyDescriptor, defineProperty and deleteProperty
@tvcutsem

This comment has been minimized.

Copy link

tvcutsem commented Aug 17, 2016

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

This comment has been minimized.

Copy link
Contributor Author

claudepache commented Sep 21, 2016

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

This comment has been minimized.

Copy link
Member

bterlson commented Nov 29, 2016

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

@tvcutsem

This comment has been minimized.

Copy link

tvcutsem commented Dec 4, 2016

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

This comment has been minimized.

Copy link
Member

bterlson commented Dec 5, 2016

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

@raulsebastianmihaila

This comment has been minimized.

Copy link

raulsebastianmihaila commented Feb 28, 2017

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

This comment has been minimized.

Copy link

tvcutsem commented Mar 12, 2017

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.

@raulsebastianmihaila raulsebastianmihaila referenced this pull request Jan 6, 2018

Open

PRs policy #1061

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Mar 5, 2018

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

@erights

This comment has been minimized.

Copy link

erights commented Sep 15, 2018

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.