1113369 - Change [[Set]] and other internal methods to support errors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

ES6 specifies that 5 internal methods ([[SetPrototypeOf]], [[PreventExtensions]], [[DefineOwnProperty]], [[Set]], [[Delete]]) return boolean values.

This is done to avoid piping an explicit 'strict' parameter throw everything in the spec. Strict callers and nonstrict callers call these methods in just the same way, but strict callers check the return value and throw a TypeError if it's false.

Instead we pass a strict parameter all over the place. In our defense this does let us generate better error messages. But we have to change it in order to support Reflect.set() and friends, which expose the boolean return values.

To do this while keeping the good error messages will require changing the boolean return value to something like a 'Maybe<ErrorMessage>'.

Comment 3

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=a3edfe22089b

Comment 4

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=51ac4ba722f3

Comment 5

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=65fff9152924

Comment 6

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=0380fd89459f

Comment 7

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=69c3555a1405

Comment 8

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=3b1c56238a46

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Introduce JS::ObjectOpResult and use it in js::StandardDefineProperty

In general, jsobj.h will offer, for each standard internal method that returns a boolean value indicating success/failure, signatures with a JS::ObjectOpResult& out-parameter and signatures without it. The ones without will throw a TypeError on failure (that is, the behavior will be "strict").

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Use JS::ObjectOpResult in js::SetArrayLength

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - [[DefineOwnProperty]]

Add an ObjectOpResult out-param for DefineProperty functions everywhere. We leave a few js::DefineProperty() convenience functions with no *result out-param. These have strict behavior: that is, they automatically check the result and throw if it is false. In bug 1125624 these strict signatures may end up being called DefinePropertyOrThrow, as that is what the spec calls it.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - [[Set]]

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - [[Delete]]

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - [[PreventExtensions]]

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - [[SetPrototypeOf]]

Comment 16

[Jason Orendorff [:jorendorff]]

By the way, if your impression after reading a bunch of this is that the many hunks like this are not helping:

>             MOZ_ASSERT(!setter);
>-            return CallAddPropertyHookDense(cx, obj, index, value);
>+            if (!CallAddPropertyHookDense(cx, obj, index, value))
>+                return false;
>+            return result.succeed();
>         }
>     }

...then I could be convinced to make OkCode the default and get rid of Uninitialized. In every case I found where I was using an uninitialized ObjectOpResult (and there were maybe 8-10 bugs like this during development) the cause was a missing result.succeed(), not a missing result.fail().

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - [[Set]]

Comment 18

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=eb991d641b2d

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8564134 [details] [diff] [review]
part 1 - Introduce JS::ObjectOpResult and use it in js::StandardDefineProperty

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

::: js/public/Class.h
@@ +66,5 @@
> + *
> + * Typical usage:
> + *
> + *     ObjctOpSuccess success;
> + *     if (!DefineProperty(cx, obj, id, ..., &success))

The & shouldn't be there any more, right?

@@ +93,5 @@
> +        return code_ == OkCode;
> +    }
> +
> +    explicit operator bool() const { return ok(); }
> +    bool operator!() const { return !ok(); }

I don't believe you need this -- default operator! will invoke an explicit operator bool.

::: js/src/jsapi.cpp
@@ +145,5 @@
> +    MOZ_ASSERT(!ok());
> +    MOZ_ASSERT(code_ != Uninitialized);
> +
> +    if (ErrorTakesIdArgument(code_)) {
> +        RootedString str(cx, IdToString(cx, id));

This produces sane results if id is a symbol, correct?  Don't want to be losing all sanity in errors if a symbol is a property name.

::: js/src/jsobj.cpp
@@ +781,5 @@
>  
> +        // XXX Temporarily break compatibility here. The right thing is to pass
> +        // result on to ArraySetLength.
> +        if (!ArraySetLength(cx, arr, id, attrs, v, false))
> +            return false;

Gonna assume a test fails with this, a later patch fixes this, and that the two patches will land together.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8564135 [details] [diff] [review]
part 2 - Use JS::ObjectOpResult in js::SetArrayLength

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

::: js/public/Class.h
@@ +121,5 @@
>  
>      /*
> +     * Report an error or warning if necessary; return true to proceed and
> +     * false if an error was reported. Call this when failure should cause
> +     * an warning if extraWarnings are enabled.

an warning

::: js/src/jsobj.cpp
@@ +778,5 @@
>              if (desc.hasWritable() && !desc.writable())
>                  attrs = attrs | JSPROP_READONLY;
>          }
>  
> +        return ArraySetLength(cx, arr, id, attrs, v, result);

That was quick.  :-)

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> > +    if (ErrorTakesIdArgument(code_)) {
> > +        RootedString str(cx, IdToString(cx, id));
> 
> This produces sane results if id is a symbol, correct?  Don't want to be
> losing all sanity in errors if a symbol is a property name.

Oh yuck, it doesn't. Good catch. I'll fix it before landing and add a test.

Also, I think I regressed JSMSG_OBJECT_NOT_EXTENSIBLE:

    js> Object.defineProperty(Object.freeze({}), "prop", {})
    typein:1:0 TypeError: prop is not extensible

D'oh. I'll fix that somehow.

All these error messages could be improved a great deal with a fairly small amount of effort: just including a brief side-effect-free description of the object, for example. Filed bug 1134253 against that.

> > +        // XXX Temporarily break compatibility here. The right thing is to pass
> > +        // result on to ArraySetLength.
> 
> Gonna assume a test fails with this, a later patch fixes this, and that the
> two patches will land together.

Yep! The test is js/src/ecma_5/extensions/proxy-array-target-length-definition.js .

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8564138 [details] [diff] [review]
part 3 - [[DefineOwnProperty]]

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

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +192,5 @@
>  
>  bool
>  DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
> +                                MutableHandle<JSPropertyDescriptor> desc,
> +                                JS::ObjectOpResult &result, bool *defined) const

Don't you need to |return result.fail()| or something in the return-true case in this method, when an Xray was passed in?

::: js/src/builtin/TypedObject.cpp
@@ +1771,2 @@
>  {
>      return ReportPropertyError(cx, JSMSG_UNDEFINED_PROP, id);

Talk to the typed-object people, make sure they want this to unconditionally (?) throw rather than just silently do nothing in strict mode.  I would mildly expect the latter, but who knows.

::: js/src/jsarray.cpp
@@ +749,2 @@
>  {
> +    if (!obj->is<ArrayObject>())

Probably worth adding an assertion that obj isn't a proxy.  I want to say the callers are in the native property-definition path such that this is the case, so this can be done, tho I'm not 100% certain.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +560,2 @@
>  {
> +    // steps 2-4

If all the step numbers are being touched, it'd be nice to make these // Step 1. and so on, fixing caps/punctuation to be consistent with step listings elsewhere.

@@ +572,3 @@
>      RootedValue trap(cx);
>      if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
>          return false;

The spec draft I'm looking at (20150220) says this should be GetMethod, which is GetProperty, then returning undefined if null/undefined was gotten, otherwise returning that after an is-callable check (which if it fails causes a TypeError to be thrown).  Please post a followup patch to add a GetMethod method and use that here -- and if search results are predictive, probably this needs to happen for *every* trap-get.

I believe this is observable: something like Object.defineProperty(new Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.

@@ +610,5 @@
> +    if (!IsExtensible(cx, target, &extensibleTarget))
> +        return false;
> +
> +    // step 17-18
> +    bool settingConfigFalse = desc.isPermanent();

This isn't really right, is it?  It looks like this bit is set if the field's present -- cool -- or absent -- not cool.  At least comment this for fixing later?

::: js/src/vm/NativeObject.cpp
@@ +1158,5 @@
> +        if (WouldDefinePastNonwritableLength(obj, index))
> +            return result.fail(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH);
> +
> +        NativeObject::EnsureDenseResult edResult;
> +        edResult = obj->ensureDenseElements(cx, index, 1);

Mild preference for

N::E edResult =
    obj->ensure...;

@@ +1230,5 @@
>          if (!cx->shouldBeJSContext())
>              return false;
>          RootedValue nvalue(cx, value);
> +
> +        // FIXME: result should be passed to NativeSet.

Fixt later in these patches?  Get the bug filed if not.

@@ +2061,5 @@
>          return true;
>      }
>  
> +    if (WouldDefinePastNonwritableLength(obj, index)) {
> +        if (strict) {

Does some followup here propagate results into this, so this isn't handling this itself?

Comment 23

[Jason Orendorff [:jorendorff]]

Attachment: part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Use JS::ObjectOpResult in js::SetArrayLength

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 8568220 [details] [diff] [review]
part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError

s/1½/1.5/ for the benefit of hg bzexport (d'oh)

Comment 26

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - [[DefineOwnProperty]]

Add an ObjectOpResult out-param for DefineProperty functions everywhere. We leave a few js::DefineProperty() convenience functions with no *result out-param. These have strict behavior: that is, they automatically check the result and throw if it is false. In bug 1125624 these strict signatures may end up being called DefinePropertyOrThrow, as that is what the spec calls it.

Comment 27

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - [[Set]]

Comment 28

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - [[Delete]]

Comment 29

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - [[PreventExtensions]]

Comment 30

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - [[SetPrototypeOf]]

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 8568220 [details] [diff] [review]
part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError

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

::: js/public/Class.h
@@ +122,5 @@
>       * Convenience method. Return true if ok() or if strict is false; otherwise
>       * throw a TypeError and return false.
>       */
> +    bool checkStrict(JSContext *cx, HandleObject obj, HandleId id) {
> +        return ok() || reportError(cx, obj, id);

I'd mildly prefer

  if (ok())
    return true;
  return reportError(...); here.

::: js/src/jsapi.cpp
@@ +146,5 @@
> +
> +    if (code_ == JSMSG_OBJECT_NOT_EXTENSIBLE) {
> +        RootedValue val(cx, ObjectValue(*obj));
> +        js_ReportValueErrorFlags(cx, JSREPORT_ERROR, code_, JSDVG_IGNORE_STACK, val,
> +                                 NullPtr(), nullptr, nullptr);

Ugh, this is turning into a bigger and bigger mess.  :-(

::: js/src/tests/js1_5/extensions/regress-365869.js
@@ +32,5 @@
>    print('test crash from bug 371292 Comment 9');
>  
>    try
>    {
> +    expect = "TypeError: can't redefine non-configurable property 5";

Hmm, the old error message seemed better to me.  Not worth a strong complaint, tho.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 8568221 [details] [diff] [review]
part 2 - Use JS::ObjectOpResult in js::SetArrayLength

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

::: js/public/Class.h
@@ +120,5 @@
>  
>      /*
> +     * Report an error or warning if necessary; return true to proceed and
> +     * false if an error was reported. Call this when failure should cause
> +     * an warning if extraWarnings are enabled.

a warning

Comment 33

[Jeff Walden [:Waldo]]

Comment on attachment 8568231 [details] [diff] [review]
part 5 - [[Delete]]

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

::: dom/base/nsGlobalWindow.cpp
@@ +827,4 @@
>  {
>    if (nsCOMPtr<nsIDOMWindow> frame = GetSubframeWindow(cx, proxy, id)) {
> +    // Fail (which means throw if strict, else return false).
> +    return result.failCantDelete();

I'd prefer seeing this comment removed.  We should get in the habit of only referring to things in result-ful terms, forcing people to understand them in our terms, and the spec's terms.  That strict mode and non-strict behave certain ways is characteristic of *those* locations -- but not of the underlying operation that's used within both.

::: dom/bindings/Codegen.py
@@ +10294,5 @@
>          def getDeleterBody(type, foundVar=None):
>              """
>              type should be "Named" or "Indexed"
> +
> +            The possibile outcomes:

possible

::: js/src/builtin/TypedObject.cpp
@@ +2073,5 @@
>          return ReportPropertyError(cx, JSMSG_CANT_DELETE, id);
>  
>      RootedObject proto(cx, obj->getProto());
> +    if (!proto)
> +        return result.succeed();

Add a test for this behavior change.  Or possibly don't make it now, and fix it in a followup patch.

::: js/src/jit/BaselineCompiler.cpp
@@ +2044,5 @@
>  typedef bool (*DeleteElementFn)(JSContext *, HandleValue, HandleValue, bool *);
> +static const VMFunction DeleteElementStrictInfo
> +    = FunctionInfo<DeleteElementFn>(DeleteElementJit<true>);
> +static const VMFunction DeleteElementNonStrictInfo
> +    = FunctionInfo<DeleteElementFn>(DeleteElementJit<false>);

I can't remember ever seeing the = on the next line in our code - bump to previous.

::: js/src/json.cpp
@@ +717,5 @@
>                          return false;
>                  } else {
>                      /* Step 2a(iii)(3). */
>                      // XXX This definition should ignore success/failure, when
>                      //     our property-definition APIs indicate that.

The previous patch likely made this doable now, right?  Should fix in a separate patch with a test.

@@ +745,5 @@
>                          return false;
>                  } else {
>                      /* Step 2b(ii)(3). */
>                      // XXX This definition should ignore success/failure, when
>                      //     our property-definition APIs indicate that.

And here.

::: js/src/proxy/Proxy.cpp
@@ +602,2 @@
>          return false;
> +    return SuppressDeletedProperty(cx, obj, id); // XXX is this necessary?

...yeah, I'm not really sure either.  Can't remember if "native iterator" also implies object nativeness or not, and skiming jsiter.cpp didn't quickly answer either.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +708,3 @@
>      RootedValue trap(cx);
>      if (!GetProperty(cx, handler, handler, cx->names().deleteProperty, &trap))
>          return false;

GetMethod again.

::: js/src/vm/ArgumentsObject.cpp
@@ +357,1 @@
>             NativeDefineProperty(cx, argsobj, id, vp, nullptr, nullptr, attrs, result);

Worth splitting this to add a MOZ_ASSERT(ignored.ok()) as well?  I would.

@@ +468,5 @@
>       * simple data property. Note that we rely on args_delProperty to clear the
>       * corresponding reserved slot so the GC can collect its value.
>       */
> +    ObjectOpResult ignored;
> +    return NativeDeleteProperty(cx, argsobj, id, ignored) &&

And here.

::: js/src/vm/Interpreter.cpp
@@ +2363,4 @@
>      RootedId &id = rootId0;
>      if (!ValueToId<CanGC>(cx, propval, &id))
>          goto error;
> +    if (!DeleteProperty(cx, obj, id, result))

Mild preference for result by the DeleteProperty call, because the result is the direct output of the method.  The id is just an input, ergo less important, ergo should go further away.

Comment 34

[Jeff Walden [:Waldo]]

Comment on attachment 8568226 [details] [diff] [review]
part 3 - [[DefineOwnProperty]]

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

::: dom/base/nsGlobalWindow.cpp
@@ +789,5 @@
>      // Spec says to Reject whether this is a supported index or not,
>      // since we have no indexed setter or indexed creator.  That means
>      // throwing in strict mode (FIXME: Bug 828137), doing nothing in
>      // non-strict mode.
> +    return result.succeed();

Shouldn't this be a failure case, per the comment?  Fix in a followup patch with a test.

::: dom/bindings/Codegen.py
@@ +10269,5 @@
>  
>                      if (found) {
> +                      return js::IsInNonStrictPropertySet(cx)
> +                             ? opresult.succeed()
> +                             : ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "${name}");

That presumable bytecode-sniffing is ugh.  Surely we can get rid of it somehow in a followup?  I hope?

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +206,1 @@
>      return true;

Don't you need to |return result.fail()| or something in the return-true case in this method, when an Xray was passed in?

::: js/src/builtin/TypedObject.cpp
@@ +1771,3 @@
>  {
>      Rooted<TypedObject *> typedObj(cx, &obj->as<TypedObject>());
>      return ReportTypedObjTypeError(cx, JSMSG_OBJECT_NOT_EXTENSIBLE, typedObj);

Again not clear to me this should be unconditional error versus result.failCantDefine() or so.

::: js/src/jsarray.cpp
@@ +749,2 @@
>  {
> +    if (!obj->is<ArrayObject>())

Again, assert it's not a proxy?

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +560,2 @@
>  {
> +    // steps 2-4

If all the step numbers are being touched, it'd be nice to make these // Step 1. and so on, fixing caps/punctuation to be consistent with step listings elsewhere.

@@ +572,3 @@
>      RootedValue trap(cx);
>      if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
>          return false;

The spec draft I'm looking at (20150220) says this should be GetMethod, which is GetProperty, then returning undefined if null/undefined was gotten, otherwise returning that after an is-callable check (which if it fails causes a TypeError to be thrown).  Please post a followup patch to add a GetMethod method and use that here -- and if search results are predictive, probably this needs to happen for *every* trap-get.

I believe this is observable: something like Object.defineProperty(new Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.

@@ +610,5 @@
> +    if (!IsExtensible(cx, target, &extensibleTarget))
> +        return false;
> +
> +    // step 17-18
> +    bool settingConfigFalse = desc.isPermanent();

This isn't really right, is it?  It looks like this bit is set if the field's present -- cool -- or absent -- not cool.  At least comment this for fixing later?

::: js/src/vm/NativeObject.cpp
@@ +1155,5 @@
> +        if (WouldDefinePastNonwritableLength(obj, index))
> +            return result.fail(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH);
> +
> +        NativeObject::EnsureDenseResult edResult;
> +        edResult = obj->ensureDenseElements(cx, index, 1);

Mild preference for

N::E edResult =
    obj->ensure...;

@@ +1229,5 @@
>          RootedValue nvalue(cx, value);
> +
> +        // FIXME: result should be passed to NativeSet.
> +        if (!NativeSet(cx->asJSContext(), obj, obj, shape, false, &nvalue))
> +            return false;

Fixt later in these patches?  Get the bug filed if not.

@@ +2139,5 @@
>          return true;
>      }
>  
> +    if (WouldDefinePastNonwritableLength(obj, index)) {
> +        if (strict) {

Does some followup here propagate results into this, so this isn't handling this itself?

::: js/src/vm/ScopeObject.cpp
@@ +1634,5 @@
>          bool found;
>          if (!has(cx, proxy, id, &found))
>              return false;
>          if (found)
> +            return result.fail(JSMSG_CANT_REDEFINE_PROP);

Add a test for this behavioral change?  Or defer it to a followup and leave it as throwing.  Whatever's faster progress.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1958,5 @@
>          if (existing_desc.hasGetterOrSetterObject() || desc.hasGetterOrSetterObject() ||
>              existing_desc.isEnumerable() != desc.isEnumerable() ||
>              (existing_desc.isReadonly() && !desc.isReadonly()))
>          {
> +            return result.failCantRedefineProp();

Add a test for the behavioral change?  Or defer to followup.

@@ +1963,3 @@
>          }
> +        if (existing_desc.isReadonly())
> +            return result.failReadOnly();

Same.

Comment 35

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> ::: dom/bindings/DOMJSProxyHandler.cpp
> Don't you need to |return result.fail()| or something in the return-true
> case in this method, when an Xray was passed in?

Yes. Done. I also changed the JS_ReportErrorFlagsAndNumber call to result.fail(), since the flags were `JSREPORT_WARNING | JSREPORT_STRICT | JSREPORT_STRICT_MODE_ERROR` (I missed it before).

> Talk to the typed-object people, make sure they want this to unconditionally
> (?) throw rather than just silently do nothing in strict mode.  I would
> mildly expect the latter, but who knows.

I'll ask. For now I just preserved the existing behavior: be strict regardless of the strict flag, even for set(). I wanted to minimize behavior changes for this stack; but I will gladly take a follow-up bug for this if the TypedObject folks tell me they want this change...

> ::: js/src/jsarray.cpp
> @@ +749,2 @@
> >  {
> > +    if (!obj->is<ArrayObject>())
> 
> Probably worth adding an assertion that obj isn't a proxy.

It turns out all the call sites pass either an ArrayObject or a NativeObject, so I just changed the parameter type to HandleNativeObject. Yay types.

> ::: js/src/proxy/ScriptedDirectProxyHandler.cpp
> If all the step numbers are being touched, it'd be nice to make these //
> Step 1. and so on, fixing caps/punctuation to be consistent with step
> listings elsewhere.

OK, but the whole file should be changed at one go; I'll file a follow-up bug and patch.

> @@ +572,3 @@
> >      RootedValue trap(cx);
> >      if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
> >          return false;
> 
> The spec draft I'm looking at (20150220) says this should be GetMethod,
> [...]I believe this is observable: something like Object.defineProperty(new
> Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.

Ugh. Yeah. This is one of those cases where the spec is wrong... in the moral sense. But yeah, I'll file a follow-up for that too..

> @@ +610,5 @@
> > +    // step 17-18
> > +    bool settingConfigFalse = desc.isPermanent();
> 
> This isn't really right, is it?  It looks like this bit is set if the
> field's present -- cool -- or absent -- not cool.  At least comment this for
> fixing later?

Right, it's a bug. The good news is, I've already written a fix. Bug 1133081, part 5, has this:
>-    bool settingConfigFalse = desc.isPermanent();
>+    bool settingConfigFalse = desc.hasConfigurable() && !desc.configurable();

For the moment, I'll just put in a comment. Ultimately we should add a dimension to the Object.defineProperty tests: try each combination on a null-handler direct proxy. I think bug 1133081 might even get us to where that would pass. Right now it would be failure city.

> Mild preference for
>
> N::E edResult =
>     obj->ensure...;

It even fits on one line.

> > +        // FIXME: result should be passed to NativeSet.
> 
> Fixt later in these patches?  Get the bug filed if not.

You know it. (Part 4.)

> > +    if (WouldDefinePastNonwritableLength(obj, index)) {
> > +        if (strict) {
> 
> Does some followup here propagate results into this, so this isn't handling
> this itself?

Ditto.

There's more for me to do here. Tomorrow!

Comment 36

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #31)
> > +    expect = "TypeError: can't redefine non-configurable property 5";
> 
> Hmm, the old error message seemed better to me.  Not worth a strong
> complaint, tho.

Yeah. Bug 1134253 asks for better error messages all around... If it's any consolation, I'm thinking Object.defineProperty on elements is not really a thing out there.

> > +     * an warning if extraWarnings are enabled.
>
> a warning

Fixed.

Comment 37

[Jeff Walden [:Waldo]]

Comment on attachment 8568229 [details] [diff] [review]
part 4 - [[Set]]

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

::: js/src/jit/IonCaches.cpp
@@ +1472,5 @@
>      // Push stubCode for marking.
>      attacher.pushStubCodePointer(masm);
>  
> +    // Unused space, to keep the same stack layout as Proxy::set frames.
> +    static_assert(sizeof(ObjectOpResult) == 4,

sizeof(int32_t) might be slightly nicer.

@@ +2172,5 @@
>      // Push stubCode for marking.
>      attacher.pushStubCodePointer(masm);
>  
> +    // Allocate result out-param.
> +    static_assert(sizeof(ObjectOpResult) == 4,

sizeof(int32_t)

@@ +2174,5 @@
>  
> +    // Allocate result out-param.
> +    static_assert(sizeof(ObjectOpResult) == 4,
> +                  "ObjectOpResult size must match size reserved by masm.Push() here");
> +    masm.Push(Imm32(ObjectOpResult::Uninitialized));

Might be nice in non-debug to not push a value, just bump the pointer.  Micro-optimization.

@@ +2211,4 @@
>      masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
>  
> +    // Test for strict failure. We emit the check even in non-strict mode in
> +    // order to pick up the warning if extraWarnings is enabled.

Ugh.  BURN IT ALL TO THE GROUND.  :-(

Maybe we can make extra warnings a property of the script and eliminate this at some point.

@@ +2437,5 @@
> +        // the stubCode pointer in order to match the layout of
> +        // IonOOLSetterOpExitFrameLayout.
> +        static_assert(sizeof(ObjectOpResult) == 4,
> +                      "ObjectOpResult size must match size reserved by masm.Push() here");
> +        masm.Push(Imm32(ObjectOpResult::Uninitialized));

So actually.  Could we add a pushObjectOpResult method or something, so we don't have this assert combo repeated every time?  This has to be combinable somehow.

Or, actually, maybe reserveObjectOpResult() or something like that, to encapsulate the uninitialized value being pushed here, that shouldn't be necessary except in debug builds.

::: js/src/jit/JitFrames.h
@@ +720,5 @@
>      uint32_t vp0_;
>      uint32_t vp1_;
>  
> +    // result out-parameter (unused for Proxy::get)
> +    JS::ObjectOpResult result_;

Hm, at some point we should split these in two if the result's unused for some things and not others.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +949,5 @@
>  
>      // step 5-6
>      RootedValue trap(cx);
>      if (!GetProperty(cx, handler, handler, cx->names().set, &trap))
>          return false;

Also GetMethod again.

@@ +951,5 @@
>      RootedValue trap(cx);
>      if (!GetProperty(cx, handler, handler, cx->names().set, &trap))
>          return false;
>  
>      // step 7

Step numbering is off throughout this method again.  Feel free to followup-fix.

@@ +1005,1 @@
>      vp.setBoolean(success);

By wrong you mean unnecessary, right?  Not clear to me how it's wrong, exactly -- there's no value being returned, so this is just setting a value into the ether, that the "other side" is looking at even when it shouldn't, right?

::: js/src/vm/GlobalObject.h
@@ +613,5 @@
>  #endif
>          RootedObject holder(cx, intrinsicsHolder());
>          RootedValue valCopy(cx, value);
> +        ObjectOpResult ignored;
> +        return SetProperty(cx, holder, holder, name, &valCopy, ignored);

We can assert ok() on success, right?

::: js/src/vm/ScopeObject.cpp
@@ +1608,5 @@
>          Rooted<DebugScopeObject*> debugScope(cx, &proxy->as<DebugScopeObject>());
>          Rooted<ScopeObject*> scope(cx, &proxy->as<DebugScopeObject>().scope());
>  
>          if (debugScope->isOptimizedOut())
> +            return result.fail(JSMSG_DEBUG_CANT_SET_OPT_ENV);

Test?  Or defer the behavior change.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +140,1 @@
>          return JS_CallFunctionValue(cx, receiver, fval, args, vp);

I might have

  if (!JS_CallFunctionValue(...))
    return false;
  result.succeed();
  return true;

but it's a nit.

Comment 38

[Jeff Walden [:Waldo]]

I'm cool with having the uninitialized initial state -- EIBTI and all.  A little more trouble, but I think perfectly reasonable trouble to have to go to.

Comment 39

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)

My comment 35 was in response to your first review of this patch (comment 22); here I'll just reply to the new comments in your second review (comment 34).

> ::: dom/base/nsGlobalWindow.cpp
> >      // Spec says to Reject whether this is a supported index or not,
> >      // since we have no indexed setter or indexed creator.  That means
> >      // throwing in strict mode (FIXME: Bug 828137), doing nothing in
> >      // non-strict mode.
> > +    return result.succeed();
> 
> Shouldn't this be a failure case, per the comment?  Fix in a followup patch
> with a test.

Already done and reviewed in the cited bug.

> ::: dom/bindings/Codegen.py
> > +                      return js::IsInNonStrictPropertySet(cx) [...]
> 
> That presumable bytecode-sniffing is ugh.  Surely we can get rid of it
> somehow in a followup?  I hope?

Yeah. Filed bug 1135993.

> ::: dom/bindings/DOMJSProxyHandler.cpp
> @@ +206,1 @@
> >      return true;
> 
> Don't you need to |return result.fail()| or something in the return-true
> case in this method, when an Xray was passed in?

Yes. But it should be result.succeed(), because we don't want the caller to treat it as failure. Fixed.

> ::: js/src/vm/ScopeObject.cpp
> >          if (found)
> > +            return result.fail(JSMSG_CANT_REDEFINE_PROP);
> 
> Add a test for this behavioral change?  Or defer it to a followup and leave
> it as throwing.  Whatever's faster progress.

Huh. Well, I just changed it back. I don't think the change is observable. IIUC, DebugScopeProxy objects are never directly exposed to script, and its sole user (Debugger.Environment) never calls DefineProperty on it.

> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> > +            return result.failCantRedefineProp();
> 
> Add a test for the behavioral change?  Or defer to followup.
> 
> > +        if (existing_desc.isReadonly())
> > +            return result.failReadOnly();
> 
> Same.

These are testable. Deferred to bug 1135997.

Comment 40

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> ::: js/src/jit/IonCaches.cpp
> > +    // Unused space, to keep the same stack layout as Proxy::set frames.
> > +    static_assert(sizeof(ObjectOpResult) == 4,
> 
> sizeof(int32_t) might be slightly nicer.

done

> @@ +2211,4 @@
> >      masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
> >  
> > +    // Test for strict failure. We emit the check even in non-strict mode in
> > +    // order to pick up the warning if extraWarnings is enabled.
> 
> Ugh.  BURN IT ALL TO THE GROUND.  :-(

Yeah. I think we can afford the extra branch. If it regresses proxy accesses, I'll take it back out and we'll just fail to generate warnings sometimes.

> Maybe we can make extra warnings a property of the script and eliminate this
> at some point.

That's a good idea.

> So actually.  Could we add a pushObjectOpResult method or something, so we
> don't have this assert combo repeated every time?  This has to be combinable
> somehow.

done

> ::: js/src/proxy/ScriptedDirectProxyHandler.cpp
> Step numbering is off throughout this method again.  Feel free to
> followup-fix.

see next comment

> >      // XXX FIXME - This use of vp is wrong. Bug 1132522 can clean it up.
> >      vp.setBoolean(success);
> 
> By wrong you mean unnecessary, right?  Not clear to me how it's wrong,
> exactly -- there's no value being returned, so this is just setting a value
> into the ether, that the "other side" is looking at even when it shouldn't,
> right?

Right. We just have different definitions of "wrong". To me, deluded is wrong.

In any case the comment and the bogus vp.setBoolean() are both deleted in patches already r+'d in bug 1132522 (they fix the step numbers too).

> ::: js/src/vm/GlobalObject.h
> @@ +613,5 @@
> >  #endif
> >          RootedObject holder(cx, intrinsicsHolder());
> >          RootedValue valCopy(cx, value);
> > +        ObjectOpResult ignored;
> > +        return SetProperty(cx, holder, holder, name, &valCopy, ignored);
> 
> We can assert ok() on success, right?

Sure.

> >          if (debugScope->isOptimizedOut())
> > +            return result.fail(JSMSG_DEBUG_CANT_SET_OPT_ENV);
> 
> Test?  Or defer the behavior change.

Backed out for the same reason as the other DebugScopeObject change.

> >          return JS_CallFunctionValue(cx, receiver, fval, args, vp);
> 
> I might have
> 
>   if (!JS_CallFunctionValue(...))
>     return false;
>   result.succeed();
>   return true;

Agreed.

Comment 41

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #40)
> Right. We just have different definitions of "wrong". To me, deluded is
> wrong.

Hm, true, true.  Well -- they're both definitions, I just seized upon the slightly wrong one when reading.  I might have used "nonsensical" or "abominable" or some other word or phrase more clearly distinguishing "incorrect" from "this is stupid why would we ever do this ahghgngh".  :-)

Comment 42

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=bea8c00a9f1c

Comment 43

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=aae0160fff8c

Comment 44

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=14707c0454ac

Comment 45

[Jeff Walden [:Waldo]]

Comment on attachment 8568233 [details] [diff] [review]
part 6 - [[PreventExtensions]]

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

::: js/src/jsapi.cpp
@@ +172,5 @@
> +JS::ObjectOpResult::reportStrictErrorOrWarning(JSContext *cx, HandleObject obj, bool strict)
> +{
> +    MOZ_ASSERT(!ErrorTakesIdArgument(code_));
> +    RootedId id(cx, JSID_EMPTY);
> +    return reportStrictErrorOrWarning(cx, obj, id, strict);

Frankly, I think I'd rather see two reporting methods, with different code in each, than pass in a bogus id and have to worry whether the downstream code ever actually uses it in a bad way.

Comment 46

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=e7535275e586

Comment 47

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=4851980aff2b

Comment 48

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=6f184b287f6d

Comment 49

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=56740a8548ac

Comment 50

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=65a8a7a152b0

Comment 51

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=831141ad4a23

Comment 52

[Jason Orendorff [:jorendorff]]

Found the bug finally. Part 3 contains this gem:

>- bool defined = false;
>+ bool defined;

It's then being used as an outparam, but the callee expects it to be pre-initialized to false. Filed bug 1137899 to make this less time consuming to track down next time.

Comment 53

[Jason Orendorff [:jorendorff]]

Attachment: All-in-one patch

Comment 54

[Jason Orendorff [:jorendorff]]

Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch

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

Hey dvander, could you take a look at the js/ipc parts here?

Comment 55

[Jason Orendorff [:jorendorff]]

dvander: Comment 0 and the comment I'm adding in js/public/Class.h are probably the best guide to what's going on here.

Comment 56

[Jason Orendorff [:jorendorff]]

Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch

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

bholley: See previous comment. This patch adds a new outparam to 5 ProxyHandler methods, which necessarily touches every silly thing in js/xpconnect, but in a pretty shallow way.

Comment 57

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=d3a064348cbf

Comment 58

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8564134 [details] [diff] [review]
part 1 - Introduce JS::ObjectOpResult and use it in js::StandardDefineProperty

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

::: js/public/Class.h
@@ +65,5 @@
> + * it also stores an error code.
> + *
> + * Typical usage:
> + *
> + *     ObjctOpSuccess success;

"Objct"

Comment 59

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=32cff941e762

Comment 60

[Jason Orendorff [:jorendorff]]

(In reply to :Ms2ger from comment #58)
> "Objct"

Fixed already -- you can see what it looks like now in the all-in-one patch.

Try server is finally green. I had to apply one more patch, now awaiting review in bug 1138059.

Comment 61

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch

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

js/ipc changes look good

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch

> WindowNamedPropertiesHandler::delete_(JSContext* aCx,

The message for JSMSG_CANT_DELETE (from failCantDelete) is a bit of an odd fit here, since it claims the prop is non-configurable, but the properties on WindowNamedPropertiesHandler are in fact very much configurable....  Not sure whether we want a different error message here or what.

This applies to the failCantRedefineProp bug 828137 adds to nsOuterWindowProxy::defineProperty too: that claims the prop is non-configurable, which is not the case.

> nsOuterWindowProxy::delete_(JSContext *cx, JS::Handle<JSObject*> proxy,

Same issue here.

>+++ b/dom/bindings/BindingUtils.h
>+ * result is the out-parameter indicating success (read it only if
>+ *     this returns true and also sets *result to true).

You mean sets *defined to true?

>+++ b/dom/bindings/Codegen.py
>@@ -10200,140 +10197,157 @@ class CGDOMJSProxyHandler_defineProperty
>+                  return js::IsInNonStrictPropertySet(cx)
>+                         ? opresult.succeed()
>+                         : ThrowErrorMessage(cx, MSG_NO_INDEXED_SETTER, "${name}");

Shouldn't this turn into a "return opresult.fail(something)" instead of the manual js::IsInNonStrictPropertySet check?  Followup bug is ok here.

My apologies for the merge issues around the unforgeable bits in this function. :(  The good news is that the merge should be simple: the UseHolderForUnforgeable(self.descriptor) block is just gone.

>+                      return js::IsInNonStrictPropertySet(cx)
>+                             ? opresult.succeed()
>+                             : ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "${name}");

Again, this seems like it should become an opresult.fail(), in a followup.

> class CGDOMJSProxyHandler_delete(ClassMethod):
>+            The possibile outcomes:

"possible"

Again, the uses of failCantDelete here in the "indexedBody" and "namedBody" bits will lead to misleading error messages.  Followup to fix that.

> DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,

The failGetterOnly bit here is a bit weird, but it's preexisting-weird.  Please file a followup on figuring out what it's trying to do and documenting it?

r=me with the above on the DOM bits.

Comment 63

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=19b8df82f525

Comment 64

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6b1ead121a
https://hg.mozilla.org/integration/mozilla-inbound/rev/48c2332309a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/da286f0f7a49
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b18c04de86c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0712a3d4b79c
https://hg.mozilla.org/integration/mozilla-inbound/rev/35f7c0795116
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85721e91692
https://hg.mozilla.org/integration/mozilla-inbound/rev/41df9affe00f

Comment 65

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3f6b1ead121a
https://hg.mozilla.org/mozilla-central/rev/48c2332309a0
https://hg.mozilla.org/mozilla-central/rev/da286f0f7a49
https://hg.mozilla.org/mozilla-central/rev/2b18c04de86c
https://hg.mozilla.org/mozilla-central/rev/0712a3d4b79c
https://hg.mozilla.org/mozilla-central/rev/35f7c0795116
https://hg.mozilla.org/mozilla-central/rev/e85721e91692
https://hg.mozilla.org/mozilla-central/rev/41df9affe00f