bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

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




JavaScript Engine: JIT
4 years ago
2 years ago


(Reporter: gkw, Assigned: Waldo)


(Blocks: 2 bugs, {regression, testcase})

regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed, firefox47 fixed, firefox48 fixed, firefox-esr45 fixed)



(1 attachment)



4 years ago
(function() {
    x = arguments;
Object.seal(x).length = 2

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

4 years ago
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.
Flags: needinfo?(jdemooij)
Created attachment 832165 [details] [diff] [review]

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.
Assignee: nobody → jdemooij
Attachment #832165 - Flags: review?(jwalden+bmo)
Flags: needinfo?(jdemooij)

Comment 3

4 years ago
Comment on attachment 832165 [details] [diff] [review]

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) {
        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.
Attachment #832165 - Flags: review?(jwalden+bmo) → review-

Comment 4

4 years ago
Setting needinfo? to :jandem as requested in-person. :)
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij) → needinfo?(jwalden+bmo)

Comment 6

4 years ago
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.
Flags: needinfo?(jwalden+bmo)


4 years ago
Flags: needinfo?(jdemooij)
Waldo, thanks for the reply. Do you suppose you can take this? :)
Flags: needinfo?(jdemooij) → needinfo?(jwalden+bmo)

Comment 8

4 years ago
I guess.
Flags: needinfo?(jwalden+bmo)


4 years ago
Assignee: jdemooij → jwalden+bmo
OS: Mac OS X → All
Hardware: x86_64 → All

Comment 9

2 years ago
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?
Flags: needinfo?(jorendorff)
Yes, that is a very likely fix! \o/
Flags: needinfo?(jorendorff)

Comment 11

2 years ago
Resolving FIXED by bug 1148652 as per comment 10.
Last Resolved: 2 years ago
status-firefox45: --- → fixed
status-firefox46: --- → fixed
status-firefox47: --- → fixed
status-firefox48: --- → fixed
status-firefox-esr45: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.