1329321 - Reflect.{defineProperty, set, setPrototype} needs to be able to return false in some cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1178638 comment 12 and following.

The actual behavior seems to be underdefined...  But presumably the idea is that we would have something on ObjectOpResult that indicates this "yeah, we should really throw, but just return false instead" state and then we would need to propagate it out to obj_defineProperty.  I guess we can basically do that via a fail() call with some value that obj_defineProperty knows about?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that we may need the same for Object.defineProperties.  It's not clear what TC39 decided there.

Comment 2

[Jason Orendorff [:jorendorff]]

To answer bz's question, yes, that sounds easy and exactly the right approach. It's not on my list of things to do. (I expected to knock this out, but that was a year ago.)

Comment 3

[Tom S [:evilpie]]

I will pick this up. I think with this we could also match Chrome's behavior in bug 1308735 for typed arrays.

Comment 4

[Chris Mills [:cmills]]

Adding ddn, just in case.

Comment 5

[Tom S [:evilpie]]

Attachment: Add a soft-fail mechanism

We can at least use this for bug 1308735. I am not sure if instead of the bit tagging like this we should maybe just cast to -intptr_t? There is also a reference to JIT code using this directly, but I could find no such code.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8979703 [details] [diff] [review]
Add a soft-fail mechanism

Review of attachment 8979703 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Class.h
@@ -139,5 @@
>          MOZ_ASSERT(code_ != Uninitialized);
>          return code_ == OkCode;
>      }
>  
> -    explicit operator bool() const { return ok(); }

Thanks for removing this!

Comment 7

[Tom S [:evilpie]]

Comment on attachment 8979703 [details] [diff] [review]
Add a soft-fail mechanism

Review of attachment 8979703 [details] [diff] [review]:
-----------------------------------------------------------------

So I think for now I am going to assert in failureCode() that SoftFailBit is not set, because that would indicate that we are trying to use that code for something, we probably should not unless we introduce some kind of warning system.

::: js/public/Class.h
@@ -139,5 @@
>          MOZ_ASSERT(code_ != Uninitialized);
>          return code_ == OkCode;
>      }
>  
> -    explicit operator bool() const { return ok(); }

Sorry, this is just a weird diff. operator bool() is now a few lines above.

Comment 8

[Tom S [:evilpie]]

Attachment: Add a soft-fail mechanism. r=?

Comment 9

[Tom S [:evilpie]]

Comment on attachment 8983341 [details] [diff] [review]
Add a soft-fail mechanism. r=?

Basically nothing changed. I just adjusted some comments. failureCode() already asserts !ok() so changing it wasn't necessary.

My last try push of this did look kind of bad: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c0ec1262ddc896a2a5996650a32f0ca347c845a&selectedJob=181121592

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8983341 [details] [diff] [review]
Add a soft-fail mechanism. r=?

Review of attachment 8983341 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe we don't need to keep the error code.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69146bf34dc6
Add a soft-fail mechanism for JS ObjectOpResult. r=jorendorff

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/69146bf34dc6

Comment 13

[Florian Scholz (Open Web Docs)]

So, this one is a bit puzzling to me ("return false in some cases" is way to vague for docs) and I would appreciate some explanations to the entire situation, so that we can document it properly.

First, this change didn't actually make it into the official spec yet, right? 
I assume this is about https://github.com/tc39/ecma262/issues/672 which is still open.

Some time ago we tried to have this throw in Nightly but that failed, so now we want to return false instead.

Bz's comment here is how I guess this would work now: https://github.com/tc39/ecma262/issues/672#issuecomment-246457609 but this code doesn't return false for me in Nightly:

Object.defineProperty(window, "gotcha", { configurable: false, value: 5 }); // Window
window.hasOwnProperty("gotcha"); // true

Am I misunderstanding? What does soft-fail mechanism mean?

Does this affect Object.defineProperty() only and not Object.defineProperties/Reflect.defineProperty or anything else?
Is this whole thing different again in strict mode?

Thanks! :)

Comment 14

[Tom S [:evilpie]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #13)
> So, this one is a bit puzzling to me ("return false in some cases" is way to
> vague for docs) and I would appreciate some explanations to the entire
> situation, so that we can document it properly.
> 
> First, this change didn't actually make it into the official spec yet,
> right? 
> I assume this is about https://github.com/tc39/ecma262/issues/672 which is
> still open.
> 
This was actually the old motivation for this change. You are right the spec does not have a "soft-fail mechanism". Everything that we make soft-fail should always be a real failure per spec. The interaction with the DOM (window) makes it even more complicated. As of right now nothing changes for window at all, because we only use this for typed arrays.
> Some time ago we tried to have this throw in Nightly but that failed, so now
> we want to return false instead.
> 
Yes, but I am not even sure we can do. I only want us to return false if we actually don't define anything, from my understand we will still define this in the |window| case.
> Bz's comment here is how I guess this would work now:
> https://github.com/tc39/ecma262/issues/672#issuecomment-246457609 but this
> code doesn't return false for me in Nightly:
> 
> Object.defineProperty(window, "gotcha", { configurable: false, value: 5 });
> // Window
> window.hasOwnProperty("gotcha"); // true
> 
> Am I misunderstanding? What does soft-fail mechanism mean?
> 
You can observe the soft-fail mechanism with typed arrays (bug 1308735)

    var arr = new Uint8Array(0);
    // out of bounds
    Reflect.defineProperty(arr, 1, {value:2}); // now returns false, was true
    Reflect.set(arr, 1, 2);                    // now returns false
    "use strict"; arr[1] = 2;                  // 2 (this should actually throws, this is the "soft" part)

Nothing changes for window right now, we need to ask bz if he can use this.
> Does this affect Object.defineProperty() only and not
> Object.defineProperties/Reflect.defineProperty or anything else?
> Is this whole thing different again in strict mode?
> 
This affects Reflect.set, Reflect.defineProperty and Object.defineProperty. In theory a few more like preventExtensions and setPrototypeOf, but we don't use the soft-fail mechanism for those at the moment.
> Thanks! :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> First, this change didn't actually make it into the official spec yet, right? 

There are two relevant changes:

1)  Make it possible for Object.defineProperty to return false.
2)  Actually use this for Window objects.

#1 is tracked by https://github.com/tc39/ecma262/issues/672 and is not in the spec yet.  It's what this bug is about, but is not observable on its own, at least for the Window case.

#2 would need a change to the HTML spec.  It corresponds to bug 1329324, not this bug.

> Some time ago we tried to have this throw in Nightly but that failed, so now we want to return false instead.

Right.

> but this code doesn't return false for me in Nightly:

Right, because bug 1329324 is not fixed yet.  This bug just added spidermonkey-side infrastructure for that to use.

> Does this affect Object.defineProperty() only and not Object.defineProperties/Reflect.defineProperty or anything else?

Reflect.defineProperty already returns false on failure.  Its behavior would not change, I assume.  That is, we want Reflect.defineProperty to continue returning false in the cases when Object.defineProperty returns false, is my understanding of the proposal at https://github.com/tc39/ecma262/issues/672#issuecomment-246386438

Object.defineProperties would probably need a change as well; I don't know whether the changes here change it.

> Is this whole thing different again in strict mode?

No.

Comment 16

[Tom S [:evilpie]]

Thanks, Boris. My previous comment was incorrect, Object.defineProperty is of course not changing, it is still going to return the object.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Er... the whole point is for Object.defineProperty to change and return false instead of the object.  That;s the proposal in https://github.com/tc39/ecma262/issues/672#issuecomment-246386438

Comment 18

[Florian Scholz (Open Web Docs)]

Thanks Boris and Tom for your detailed explanations!

> #1 is tracked by https://github.com/tc39/ecma262/issues/672 and is not in the spec yet.  It's what this bug is about, but is not observable on its own, at least for the Window case.

> This bug just added spidermonkey-side infrastructure for that to use.

Alright, removing dev-doc-needed for this bug then.


> #2 would need a change to the HTML spec.  It corresponds to bug 1329324, not this bug.

That bug has ddn added, so lets document this there when fixed.


> You can observe the soft-fail mechanism with typed arrays (bug 1308735)

I've added a note about this web facing change, see https://bugzilla.mozilla.org/show_bug.cgi?id=1308735#c6

Comment 19

[Tom S [:evilpie]]

I kind of misused this bug to implement something that is not in spec, or what Boris was talking about. This is now going to happen in bug 1496475.

What this bug did: Added a way to "return false" from Reflect.defineProperty, Reflect.set and Reflect.setPrototype, indicating that an some property operation failed. However usually in such cases Object.defineProperty would throw, which doesn't happen, because we treat it like the defineProperty actually succeeded.