1148970 - Assertion failure: Argument check fail., at jit/MacroAssembler.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

try {
    a = new Uint8ClampedArray
    b = Object.defineProperty(this, "c", {
            get: function() {
                Number(b)
            }
        })
    b.toString = function() {
                    Int8Array(ArrayBuffer(), {
                        valueOf: function() {
                            d[2]
                        }
                    })
                 }
    e = new Number()
    Object.defineProperty(this, "d", {
        get: function() {
            return a.subarray(e)
        }
    })
    Array.call(c)
    c
} catch (e) {}
e.__proto__ = (function() {})
for (p in c) {}

asserts js debug shell on m-c changeset 385840329d91 with --fuzzing-safe --no-threads --ion-eager --ion-inlining=off at Assertion failure: Argument check fail., at jit/MacroAssembler.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 385840329d91

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1926f1d553bb
user:        Jeff Walden
date:        Tue Mar 03 11:29:49 2015 -0800
summary:     Bug 1139769 - Self-host %TypedArray%.prototype.subarray.  r=till

Setting s-s as a start because this concerns TypedArrays.

Waldo, is bug 1139769 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x136877, 0x0000000101edccc2, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x0000000101edccc2
    frame #1: 0x00000001004fa3fb js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::jit::IonCannon(JSContext*, js::RunState&) [inlined] EnterIon(data=0x0000000101ecf7c0) + 344 at Ion.cpp:2390
    frame #2: 0x00000001004fa2a3 js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::jit::IonCannon(cx=0x0000000101edcaf6, state=0x00007fff5fbfc100) + 291 at Ion.cpp:2472
    frame #3: 0x000000010020d26a js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::RunScript(cx=0x00000001028a5180, state=0x00007fff5fbfbfc8) + 202 at Interpreter.cpp:400
    frame #4: 0x00000001001fd02d js-dbg-64-dm-nsprBuild-darwin-385840329d91`js::Invoke(cx=0x00000001028a5180, args=CallArgs at 0x00007fff5fbfc050, construct=<unavailable>) + 733 at Interpreter.cpp:489
(lldb)

Comment 2

[Jeff Walden [:Waldo]]

This isn't a bug introduced by my patch -- only a bug uncovered by it.  Reducing awhile gets this testcase entirely omitting any use of %TypedArray%.prototype.subarray:

  Uint8Array.prototype.mysub = function(begin) { begin | 0; };

  var arr = new Uint8Array();
  var begin = new Number(0);

  function q()
  {
    return arr.mysub(begin);
  }

  q();
  q();

  begin.__proto__ = parseInt;

  q();

*Something* about typeset information, or inlining, or something, isn't being properly updated when the prototype mutation happens.  What, I don't know.  But this most definitely is not a subarray bug.  jandem, maybe you can take a look?  The testcase is small enough it's probably even something that can be eyeballed, at this point.

Comment 3

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

Using the testcase in comment 2,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5cec093aeadc
user:        Brian Hackett
date:        Wed Jan 14 08:00:28 2015 -0700
summary:     Bug 1116017 - Don't scan all type sets in compartments on type mutations, r=jandem.

Brian, is bug 1116017 a likely regressor?

Comment 4

[Jan de Mooij [:jandem]]

Yes that bisection makes sense I think, forwarding to bhackett.

Comment 5

[Andrew McCreight [:mccr8]]

Sounds kind of scary so I'm going to put it as sec-high.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

This is another spot where our DEBUG type checking code was not updated to handle the change in type set semantics from bug 1116017.

Comment 7

[Brian Hackett [Laid off!]]

This is only a bug in our debugging checks, not s-s.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8588637 [details] [diff] [review]
patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +3544,5 @@
> +                Label skip;
> +                Address addr(StackPointer, ArgToStackOffset((i - info.startArgSlot()) * sizeof(Value)));
> +                masm.branchTestObject(Assembler::NotEqual, addr, &skip);
> +                Register obj = masm.extractObject(addr, temp);
> +                masm.guardTypeSetMightBeIncomplete(obj, temp, &success);

I think this should jump to "skip" (should be renamed then) instead of "success" (if the first succeeds we still want to check the other types), followed by a masm.jump() to the assumeUnreachable case below.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> I think this should jump to "skip" (should be renamed then) instead of
> "success" (if the first succeeds we still want to check the other types),
> followed by a masm.jump() to the assumeUnreachable case below.

We're only at this spot (after the |miss| label) if the argument check missed for one of the arguments.  We don't know which argument, so this code goes through them all and sees if there is any object passed in which could have caused the argument check to fail.  This modification isn't perfect, but I don't see a non-invasive way of keeping track of which argument we failed to type check.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8588637 [details] [diff] [review]
patch

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

Oops, my bad. I misunderstood the label argument of guardTypeSetMightBeIncomplete.

Comment 11

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdd47cc88a6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cdd47cc88a6