858677 - Give ObjectOps::delete* a JSBool* outparam for callers to use to determine whether to throw on failure-to-delete

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

[[Delete]] in ES6 returns true or false -- false if there was an own, non-configurable property, true otherwise.  Callers (like strict mode code) that want to throw in the former case, simply do so.  We should change our deletion interfaces to act similarly.  This will fix some issues with proxy code (which has some comments about ignoring strictness arguments).  It'll also reduce API mismatches, as most of the deletion APIs currently have a Value outparam to indicate this true/false return value.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - JS engine parts

Random-ish notes from a patch-skim before posting for review:

A whole lotta this is just switching JSClass definitions to not use JS_PropertyStub for JSClass::delProperty.  Another whole lot is just changing implementations of the delete operations to not take rval/strict but rather just JSBool*.  A little bit is updating all the JSAPI method implementations to work with this -- which, surprisingly, they do, because they already expose the delete-succeeded or delete-threw thing as their only mode of output.  (Not often does JSAPI get the mode of interaction right when the internals get it wrong!)

The only kinda-interesting parts are the jsarray.cpp changes.  DeleteArrayElement's interface got a lot nicer, I think.  But most stuff wanted DeletePropertyOrThrow (name comes direct from ES6), so I introduced that and switched most places to it.  The big issue was SetOrDeleteArrayElement, whose interface doesn't deal well with the succeeded thing.  ES6 bifurcates hole/non-hole, so I did the same at all the call sites and got rid of SODAE entirely.  Note that array_reverse, if you compare against ES6, was performing low-element, high-element deletes/gets in the high/low order, not the specified low/high order.  I switched this when rewriting this and added a test for it.

This changes the semantics of JSClass::delProperty -- *succeeded must be set on return-true (on return-false it doesn't matter as that's an error case).  I think this is a good change, but it is a change to note.  The JSAPI signature change should mean everyone will be forced by the type system to switch signatures, so they'll be touching every implementation already -- making sure succeeded gets set properly shouldn't be hard, then.

The JSON algorithms specifically ignore the result of deletion, in case you read this before you look at those changes and start wondering if they're right.

Note that js/src/jit-test/tests/basic/bug821850.js was specifically buggy -- it should have been complaining about deleting a non-configurable property.  This was due to the XXX comment in Proxy::delete_, which is happily gone now that the proxy API matches the ObjectOps delete API.  (Well, modulo bool/JSBool.  Which is sad, but the JITs really need to learn about bool and bool* and all those in a separate patch, before that could change.  :-) )

There's a slight bit more cleaning that could be done here: DeleteNameOperation takes an rval outparam, when it really could use a JSBool* one for consistency.  Also DeleteElement and DeleteProperty shouldn't have strict variants -- that should be the responsibility of the calling JIT code, to throw on strict failure or compute the right boolean result.  So there's some oddness to those methods, now.  But that's JIT stuff, and changing that is totally dilatory here.  :-)

A shell builds with all these changes and passes jit-tests and jstests.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - Changes outside the JS engine

The ObjectWrapperParent sadness here is just about the only bit of this that's even the faintest bit interesting, I think.  There's a little bit in the NPObject bits to make deletion return stuff correctly, and there's some CPOW ObjectParentWrapper additions, but otherwise this is basically straightforward.

The first patch is independent of the second, from the standalone JS engine build point of view.  Both are codependent from Gecko's point of view, tho, so expect both to land as one patch, when reviewed.

Comment 3

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=cfa027dee36b is green save for consistent JP orange, and that failure clearly implicating this change (ergo not pre-existing permaorange).  I'll take a look into that and figure out what's the matter in a separate/followup patch, to land at the same time as these patches.

Comment 4

[Jeff Walden [:Waldo]]

The relevant jetpack test code excerpt is from test/test-environment.js:

  const { get, set, exists } = Cc['@mozilla.org/process/environment;1'].
                               getService(Ci.nsIEnvironment);

  exports['test unset'] = function(assert) {
    env.BLA4 = 'bla';
    assert.equal(env.BLA4, 'bla', 'BLA4 env varibale is set');
    delete env.BLA4;
    assert.equal(env.BLA4, undefined, 'BLA4 env variable is unset');
    assert.equal('BLA4' in env, false, 'BLA4 env variable no longer exists' );
  };

The env value is this, again excerpted:

  exports.env = Proxy.create({
    // New environment variables can be defined just by defining properties
    // on this object.
    defineProperty: function(name, { value }) set(name, value),
    delete: function(name) set(name, null),
  });

Function closure syntax evaluates and returns the expression from the closure -- that is, set(name, ...).  Looking to nsIEnvironment, we find this:

  /**
   * Set the value of an environment variable.
   *
   * @param aName   the variable name to set.
   * @param aValue  the value to set.
   */
  void set(in AString aName, in AString aValue);

Setting aside that the proxy here is a hated indirect proxy ;-) the delete proxy hook should return a value to be coerced to true or false, corresponding to the effective value of the [[Delete]] operation being hooked.  (The same might be true for defineProperty wrt [[DefineOwnProperty]], although I'm not as sure of this for indirect proxies.)  This delete hook returns the result of set(), which will be |undefined|.  That corresponds to |false|, i.e. the [[Delete]] operation failed, without throwing an error.  Before the changes in the patches here, the proxy code silently discarded Throw parameters and so on, so the only result of this would be that |delete env.BLA4| would evaluate to false, big whoop.  After...well, test-environment.js is strict mode code.  And that means a failing delete operation throws a TypeError.  I'm not quite sure what the semantics of defineProperty in old proxies are, but whenever this switches to direct proxies, returning the result of set() will have identically bad behaviors (undefined/false corresponding to "the property couldn't be defined", ergo TypeError in strict mode).

...oh hey, Eddy!  Just the person I was looking for, fancy seeing you here.  ;-)  How do I get the Jetpack code not misusing proxies for delete (and perhaps defineProperty too), and inhibiting bug fixing in proxy code?  Any chance you might be able to push through a fix to the code, and then get that Jetpack version into mozilla-central, so I can move forward with the rest of this?

Comment 5

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

Comment on attachment 734192 [details] [diff] [review]
1 - JS engine parts

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

::: js/src/methodjit/StubCalls.cpp
@@ +1365,3 @@
>              THROW();
> +        MutableHandleValue rval = MutableHandleValue::fromMarkedLocation(&f.regs.sp[-1]);
> +        rval.setBoolean(true);

Not succeeded?

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 734192 [details] [diff] [review]
1 - JS engine parts

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

::: js/src/methodjit/StubCalls.cpp
@@ +1365,3 @@
>              THROW();
> +        MutableHandleValue rval = MutableHandleValue::fromMarkedLocation(&f.regs.sp[-1]);
> +        rval.setBoolean(true);

Hm, right.  I was thinking it didn't matter because |delete name| always returns true, but that's not true if |name| was a non-eval var, or resolves to a global property that's non-configurable.

I'll make the change, but in any case it hardly matters, as with methodjit disabled now, it's not even possible to invoke this code.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 734193 [details] [diff] [review]
2 - Changes outside the JS engine

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

r=bholley on everything but the ipc stuff. Flagging billm for that.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 734193 [details] [diff] [review]
2 - Changes outside the JS engine

We should just delete the current IPC code. It's totally broken and the new implementation shares almost no code with it. So whatever we do there is fine.

Comment 9

[Eddy Bruel [:ejpbruel]]

The fact that Jetpack misuses the delete trap is something that should just be fixed, along with the fact that it still uses indirect proxies.

Gozala, how hard would it be to fix this code? Could we quickly push a fix to Jetpack so Waldo can continue?

Comment 10

[Jeff Walden [:Waldo]]

I filed bug 861304 for comment 4's issue, with a patch.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 734192 [details] [diff] [review]
1 - JS engine parts

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

This was surprisingly straightforward, just a lot of code to read. Nice.

::: js/src/jit-test/tests/basic/bug821850.js
@@ +6,4 @@
>  
>  x = new Proxy(m, {})
>  assertEq(x.p, 3);
> +assertThrowsInstanceOf(function fun() {"use strict"; return delete x.p; }, TypeError);

Yay!

::: js/src/jsapi.h
@@ +841,5 @@
> + * If an error occurred, return false as per normal JSAPI error practice.
> + *
> + * If no error occurred, but the deletion attempt wasn't allowed (perhaps
> + * because the property was non-configurable), return true and set *succeeded
> + * to false.  This will cause |delete obj[id]| to evaluate to false in

Style micro-nit: Here and in the following paragraph, I'd reverse the wording:
  "set *succeeded to true/false and return true."

::: js/src/jsarray.cpp
@@ +511,5 @@
> +            //     property we find, as we do in the simple-loop case above.
> +            //     But since we're iterating in unknown order here, we don't
> +            //     know if we're hitting the highest non-configurable property
> +            //     when we hit a failure.  For now just drop unsuccessful
> +            //     deletion on the floor, as the previous code here did.

Bluh. Please file the follow-up bug on this goofy pre-existing behavior and cite it in this comment. Things to mention:

1. Maybe it should go without saying but optimizations should be transparent. I claim the second path here should only be allowed to exist if we care enough to make an array of indices and sort it.  >:-P

2. (much less important) The heuristic seems silly to me; we should be comparing the amount of work to use approach A vs. approach B, rather than approach A vs. a magic constant.

@@ +1079,5 @@
> +            if (!SetArrayElement(cx, obj, len - i - 1, lowval))
> +                return false;
> +        } else {
> +            // No action required.
> +        }

Deleting SetOrDeleteArrayElement made every place that used it wordier. I vote for restoring it. I understand deleting it makes the code look more like the spec. Your call.

@@ +1921,1 @@
>              return JS_FALSE;

Could you please s/JS_FALSE/false/g in this file, and s/JS_TRUE/true/g, in a separate changeset? They are already mostly gone; now is as good a time as any.

rs=me with a

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Style micro-nit: Here and in the following paragraph, I'd reverse the
> wording:
>   "set *succeeded to true/false and return true."

Fine by me.

> Bluh. Please file the follow-up bug on this goofy pre-existing behavior and
> cite it in this comment. Things to mention:
> 
> 1. Maybe it should go without saying but optimizations should be
> transparent. I claim the second path here should only be allowed to exist if
> we care enough to make an array of indices and sort it.  >:-P

I actually did this in attachment 737554 [details] [diff] [review] for bug 858381 -- the existing code's so far from the spec algorithm that it seemed right (and really not hard) to do it during what was essentially a rewrite.  I poked that review at bhackett because the patch had a bit to do with ObjectElements field invariant details, but feel free to snag yourself a review on it if you want.

> 2. (much less important) The heuristic seems silly to me; we should be
> comparing the amount of work to use approach A vs. approach B, rather than
> approach A vs. a magic constant.

What we *should* do is store elements (even sparse elements) separate from all other properties, like v8 does.  Then, assuming the store allows linear-time, in-order traversal (v8 does), deleting elements top to bottom requires no heuristics.

That patch doesn't have such a comment in it right now; I'll add it.

> Deleting SetOrDeleteArrayElement made every place that used it wordier. I
> vote for restoring it. I understand deleting it makes the code look more
> like the spec. Your call.

I think it's a little clearer this way, but I see your point.  If property-setting had the same interface as property deletion -- that is, return true or false to indicate whether the property set was disallowed -- I might be up for the reunification.  Bug 826587 went a fair ways down that road but didn't get all the way there.  When that happens, reconsidering seems appropriate.

> Could you please s/JS_FALSE/false/g in this file, and s/JS_TRUE/true/g, in a
> separate changeset? They are already mostly gone; now is as good a time as
> any.

I'll do this after bug 858381 -- I probably touch the file enough there that it's less pain doing it after that, than before.  :-)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4925a84c57cf
https://hg.mozilla.org/mozilla-central/rev/a79abb6f9f8a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b26c4651a1f