937922 - Differential Testing: Different output message involving Object.seal, arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
    x = arguments;
})()
Object.seal(x).length = 2
print(x.length)

when run with --fuzzing-safe --ion-eager, shows "2" in stdout, without the flag, shows "0".

Tested on a 64-bit js dbg more-deterministic shell on m-c changeset 16949049f03d.

My configure flags are:

sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --disable-threadsafe

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This goes back as far as http://hg.mozilla.org/mozilla-central/rev/541248fb29e4 but I hit other bisection issues trying to go back further on Mac, so let's just call this at least a 5 month old issue for now.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

The interesting part is this:

    Object.seal(x).length = 2;

The problem is that ArgSetter calls DeleteGeneric + DefineGeneric and relies on args_delProperty calling markLengthOverridden. However, if the object is sealed, DeleteGeneric will fail and never call args_delProperty but the DefineGeneric *will* override arguments.length.

This patch adds a markLengthOverridden call to ArgSetter and StrictArgSetter.

I wondered if args.callee needs the same treatment, but I couldn't come up with a testcase that fails so I left it alone for now.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 832165 [details] [diff] [review]
Patch

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

Deleting the .length from a frozen arguments object should be exactly like deleting any value property from a frozen object.  In strict mode, the deletion attempt should throw.  Outside it, it should do nothing.  If the deletion fails, we shouldn't even get to the definition!  The right fix is to *not* chain together the deletion and the definition in a single expression, about like so:

  bool succeeded;
  if (!baseops::DeleteGeneric(..., &succeeded))
      return false;
  if (!succeeded) {
    if (strict) {
        obj->reportNotConfigurable(...);
        return false;
    }
    return true;
  }

  return baseops::DefineGeneric(...);

::: js/src/jit-test/tests/basic/bug937922.js
@@ +13,5 @@
> +    var x;
> +    (function() {
> +	x = arguments;
> +    })();
> +    Object.seal(x).length = 2;

This should totally be throwing a TypeError, per later comments.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting needinfo? to :jandem as requested in-person. :)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Deleting the .length from a frozen arguments object should be exactly like
> deleting any value property from a frozen object.

Should |arguments.length = 3;| also throw in strict mode if |arguments| is sealed instead of frozen? I thought the Delete + Define was an implementation quirk but if the Delete is indeed supposed to happen per-spec and should throw in this case I can do what you suggested in comment 3.

Comment 6

[Jeff Walden [:Waldo]]

Oh!  You're right.  Sealing makes an object non-extensible and marks all properties non-configurable.  But it leaves writable properties writable.  As arguments.length is writable, the write should still work here.

But, it's not okay to implement arguments.length writing by deletion+redefining, in that case, because deletion shouldn't work here, and redefinition should fail because the object is non-extensible.

Looking at this harder, it's not clear to me why we can't just create the arguments object with a length property on it from birth, using a reserved slot to store the length.  Some care may be required for deletion/redefinition of the length property (in script, not in StrictArgsSetter that we would no longer need) to work properly, but it doesn't seem like it'd be too bad.  We've done all this for objects before, actually, so there's precedent for it.  Not sure if that precedent included adding configurable properties this way or not, but I /think/ it did.

Comment 7

[Jan de Mooij [:jandem]]

Waldo, thanks for the reply. Do you suppose you can take this? :)

Comment 8

[Jeff Walden [:Waldo]]

I guess.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5147517c8a18
user:        Jason Orendorff
date:        Mon Mar 23 14:32:33 2015 -0500
summary:     Bug 1148652, part 3 - Mark arguments.length as overridden when it is redefined via the C API. r=efaust.

Jason, is bug 1148652 a likely fix?

Comment 10

[Jason Orendorff [:jorendorff]]

Yes, that is a very likely fix! \o/

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Resolving FIXED by bug 1148652 as per comment 10.