1085299 - Differential Testing: Different output message involving Object.preventExtensions

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect, P5)

Description

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

x = y = new Uint8Array
z = ArrayBuffer(function() {
    "use strict";
    x = function() {
        y[0] = 0;
    };
}());
z.toSource = function() {
    return function() {
        x();
    };
}();
Object.preventExtensions(y);
try {
    0 instanceof z
} catch (e) {}
try {
    print(uneval(z))
} catch (e) {}


$ ./js-dbg-opt-32-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --ion-eager testcase.js
undefined
$

$ ./js-dbg-opt-32-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --baseline-eager 61797.js
$

Tested this on m-c rev 33c0181c4a25.

My configure flags are:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/78fa90a29c43
user:        Brian Hackett
date:        Tue Mar 04 12:42:08 2014 -0700
summary:     Bug 695438 - Make typed arrays native objects, allow adding new named properties, r=luke.

Brian, is bug 695438 a likely regressor?

Comment 1

[Brian Hackett [Laid off!]]

Attachment: disallow extensions on typed arrays

IonMonkey has an optimization where it ignores out of bounds writes on typed arrays.  When the array is non-extensible and we're in strict mode, the interpreter throws on such assignments.  I looked at how v8 addresses this, and it just throws when calling Object.preventExtensions on a typed array, so that's what this patch does too.

Comment 2

[Luke Wagner [:luke]]

From my reading of
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-integerindexedelementset
I don't think we should ever throw when assigning to an integer property.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8510331 [details] [diff] [review]
disallow extensions on typed arrays

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

This isn't what ES6 says is supposed to happen.  ES6 is perfectly willing to let you call [[PreventExtensions]] on a typed array.  See how there's no special definition of [[PreventExtensions]] for typed arrays, which means they pick up the ordinary definition of the method (that changes [[Extensible]] to false and returns true).

I'm not certain what's supposed to happen for indexed sets on a non-extensible typed array in strict mode code, but I think it's supposed to be an exception gets thrown.  See the "If index < 0 or index ≥ length, then return false." step.  The false return value passes to, and is returned by, the caller.  If that's [[Set]] or [[DefineOwnProperty]], that corresponds to the operation not being successful.  For strict mode code, that causes the calling code to throw.  (It also causes Object.defineProperty to throw, same idea.)

But to be sure, I'm not 100% certain what's claimed to be failing with this testcase, or how the proposed change here manages to fix it (other than by short-circuiting the entire thing).  I can't reproduce different behavior with a current shell on the copy-pasted testcase.  And if I roll back to the quoted revision, I can't reproduce it either.  64-bit, debug, no optimizations, both cases.  (Given no mention of 32-bit anywhere here, and my not having a 32-bit setup at the moment, I'm disinclined to investigate that possibility unless specifically told it's relevant, from past pain in trying to set up a cross-compile system.)  It also looks suspicious that the two shell commands/outputs here, use *different* file names: testcase.js versus 61797.js.

Anyway.  In order for me to r+ anything here, I'd like a testcase that *actually* behaves differently with the two flags and does the minimal set of things to trigger an error, and I want to see a fix that's in line with the ES6 spec.  This doesn't seem to do either.

Comment 4

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> But to be sure, I'm not 100% certain what's claimed to be failing with this
> testcase, or how the proposed change here manages to fix it (other than by
> short-circuiting the entire thing).  I can't reproduce different behavior
> with a current shell on the copy-pasted testcase.  And if I roll back to the
> quoted revision, I can't reproduce it either.  64-bit, debug, no
> optimizations, both cases.  (Given no mention of 32-bit anywhere here, and
> my not having a 32-bit setup at the moment, I'm disinclined to investigate
> that possibility unless specifically told it's relevant, from past pain in
> trying to set up a cross-compile system.) 

It is 32-bit-specific, I just retested. I couldn't reproduce on 64-bit either.

> It also looks suspicious that the
> two shell commands/outputs here, use *different* file names: testcase.js
> versus 61797.js.

Sorry, I screwed up the naming during reporting. Both refer to the same file. In any case, here's the retest:

$ ./js-dbg-opt-32-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --baseline-eager 1085299.js

$ ./js-dbg-opt-32-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --ion-eager 1085299.js
undefined

Comment 5

[Brian Hackett [Laid off!]]

Here's a better testcase that shows the problem on both x86 and x64 (run with no options vs. with --ion-eager --no-threads):

function writeArrayBuffer(x) {
    x[0] = 0;
}
function strictWriteArrayBuffer(x) {
    "use strict";
    x[0] = 0;
}
with({}){}
for (var i = 0; i < 10; i++) {
    var buf = new Int32Array(0);
    if (i == 9)
        Object.preventExtensions(buf);
    writeArrayBuffer(buf);
    strictWriteArrayBuffer(buf);
}

Waldo, what do you think of Luke's reading in comment 2?  Throwing an exception here will entail adding some new deoptimizations that only take effect in strict mode code, which are in general pretty annoying to deal with and it would be good if we didn't have more of them.

Comment 6

[Jeff Walden [:Waldo]]

I don't think his reading is accurate.  :-)

But the solution isn't to just ignore the spec -- it's to complain upstream and get the spec changed, then deal with that reality here.  Particularly, I'd thought there was general agreement that out-of-bounds accesses and sets would act independent of the prototype chain as no-ops, with gets returning |undefined|.  It looks like IntegerIndexedElementGet does indeed act this way.  But IntegerIndexedElementSet returns false in this case -- which directly feeds into the return value of [[Set]] and [[DefineOwnProperty]], causing a strict mode user to throw.  So the spec isn't in line with expectations.

So what we should do is file a TC39 bug to make IntegerIndexedElementSet's "If index < 0 or index ≥ length, then return false." step instead read, "If index < 0 or index ≥ length, then return true."  And we should make out-of-bounds element-setting on a non-extensible typed array simply do nothing, not throw.  But making Object.preventExtensions deviate from the spec on typed arrays only compounds the errors here.

Comment 7

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

As per quick in-person chat, setting needinfo? from :Waldo to move this forward. (maybe upstream?)

Comment 8

[Jeff Walden [:Waldo]]

Started the es-discuss thread:

https://mail.mozilla.org/pipermail/es-discuss/2015-February/041769.html

Pile on, add context, etc. now.

Comment 9

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

I just retested this and:

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --no-threads --ion-eager 1085299.js
1085299.js:2:5 TypeError: calling a builtin ArrayBuffer constructor without new is forbidden
Stack:
  @1085299.js:2:0

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --no-threads --baseline-eager 1085299.js
1085299.js:2:5 TypeError: calling a builtin ArrayBuffer constructor without new is forbidden
Stack:
  @1085299.js:2:0

I changed the testcase to:

x = y = new Uint8Array
z = new ArrayBuffer(function() {
    "use strict";
    x = function() {
        y[0] = 0;
    };
}());
z.toSource = function() {
    return function() {
        x();
    };
}();
Object.preventExtensions(y);
try {
    0 instanceof z
} catch (e) {}
try {
    print(uneval(z))
} catch (e) {}

and got:

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --no-threads --ion-eager 1085299.js
undefined

$ ./js-dbg-64-dm-clang-darwin-d5d53a3b4e50 --fuzzing-safe --no-threads --baseline-eager 1085299.js
undefined


Waldo mentioned over IRC that "allegedly we agreed to whatever the spec says, and we just need to implement it". Since we're not seeing a difference with the testcase, I'll take this off compareJIT's ignore list for now.

Comment 10

[Till Schneidereit [:till]]

Jeff, I'm not sure my reading of the discussion is correct, so can you confirm whether our behavior is as expected?

Comment 11

[Jeff Walden [:Waldo]]

Our behavior still disagrees with what TC39/ECMA says should happen here.  Out-of-bounds sets still fail the "If numericIndex >= length, return false" step of https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownproperty-p-desc which in strict mode code turns into a thrown exception.  We still don't have a throwing path in any of this, unless I'm horribly mistaken.  So this is still an issue.

I heard a rumor that Edge implemented (and shipped?) proper semantics here, but I can't say that with 100% confidence.  Worth trying to check with bterlson on that point perhaps.

Comment 12

[André Bargull [:anba]]

Issue no longer reproduces for me -> WFM.