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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: bzbarsky, Assigned: evilpie)

Tracking

Trunk
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

()

Attachments

(3 attachments)

+++ 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.
Flags: needinfo?(evilpies)
Posted patch WIPSplinter Review
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.
Assignee: nobody → evilpies
Flags: needinfo?(evilpies)
If we also want to fix Object.defineProperties we can just copy some the same code from intrinsic_DefineProperty to that function.
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.
Attachment #9014464 - Flags: feedback?(bzbarsky)
Comment on attachment 9014464 [details] [diff] [review]
WIP

As far as I can tell, this does the right thing, thank you!
Attachment #9014464 - Flags: feedback?(bzbarsky) → feedback+
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?
For purposes of Window, that case should not matter.
Depends on D12948
Pushed by evilpies@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/5da532fab9ba
https://hg.mozilla.org/mozilla-central/rev/6b2aceb0979a
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.