Closed Bug 1496475 Opened 4 years ago Closed 4 years ago

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


(Core :: JavaScript Engine, enhancement, P3)




Tracking Status
firefox64 --- wontfix
firefox65 --- fixed


(Reporter: bzbarsky, Assigned: evilpie)





(3 files)

+++ 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)
Attached 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]

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]

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:

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
Object.defineProperty needs to be able to return false when trying to define a non-configurable property on a WindowProxy. r=jorendorff
JSAPI test. r=jorendorff
Closed: 4 years 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.