1496475 - Object.defineProperty needs to be able to return false in some cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

+++ This bug was initially created as a clone of Bug #1329321 +++

See bug 1178638 comment 12 and following.

Per IRC discussion today, bug 1329321 didn't implement what I need for bug 1329324.  What is needed are the following semantics:

1)  A way for WindowProxy to set a state on an ObjectOpResult that is treated as failure by everything except Object.defineProperty (and maybe Object.defineProperties, but that's not clear yet).

2)  When this state is set, intrinsic_DefineProperty should probably pretend like "strict" is false.  Need to check whether it can end up with "strict" being true when called from something other than Object.defineProperty.

3)  intrinsic_DefineProperty should return false when this state is set.

4)  When ObjectOrReflectDefineProperty returns false to ObjectDefineProperty, ObjectDefineProperty itself should return false (or maybe null?  Needs another TC39 discussion to change it to null).

Let's not worry about Object.defineProperties for now.  If we end up needing to  change that, it wouldn't be hard to make it call the ObjectOpResult version of js::DefineProperty and deal accordingly.  But for now we have no evidence that its behavior needs changing.

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

This adds a new special method ObjectOpResult::failCantDefineWindowNonConfigurable, which is treated like a normal failure everywhere but the intrinsic _DefineProperty function when in strict mode. This basically means Object.defineProperty, but currently also __define{G,S}etter__.

I haven't tested this.

Comment 2

[Tom S [:evilpie]]

If we also want to fix Object.defineProperties we can just copy some the same code from intrinsic_DefineProperty to that function.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 9014464 [details] [diff] [review]
WIP

Just ping me when you verified that this approach works for you.

I need to think about how to test this on our side. I think we already have some jsapi-tests that implement a custom proxy, so I will probably go that route.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9014464 [details] [diff] [review]
WIP

As far as I can tell, this does the right thing, thank you!

Comment 5

[Tom S [:evilpie]]

We occasionally call a different function _DefineDataProperty: https://searchfox.org/mozilla-central/rev/6d748c47a82f59d75c22bf301177ad8fb10f786b/js/src/builtin/Object.js#262-270.

I think for this case it might not really matter, because we only care about {configurable: false} case?

Comment 6

[Boris Zbarsky [:bzbarsky]]

For purposes of Window, that case should not matter.

Comment 7

[Tom S [:evilpie]]

Attachment: Bug 1496475 - Object.defineProperty needs to be able to return false when trying to define a non-configurable property on a WindowProxy. r?jorendorff

Comment 8

[Tom S [:evilpie]]

Attachment: Bug 1496475 - JSAPI test. r?jorendorff

Depends on D12948

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5da532fab9ba
Object.defineProperty needs to be able to return false when trying to define a non-configurable property on a WindowProxy. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/6b2aceb0979a
JSAPI test. r=jorendorff

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5da532fab9ba
https://hg.mozilla.org/mozilla-central/rev/6b2aceb0979a