1001378 - Assertion failure: isObject(), at js/Value.h:1157 or Crash [@ aliasedVarFromArguments]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 5ecd532a167e (run with --fuzzing-safe --ion-eager --ion-eager):


function boo() { 
  return foo.arguments[0];
}
function foo(a,b,c) { 
  if (a == 0) {
    a ^= ""; 
    return boo();
  } 
}
function inlined() { 
  return foo.apply({}, arguments); 
}
inlined(1,2,3);
inlined(0,2,3);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/d34458e80bcb
user:        Shu-yu Guo
date:        Thu Apr 24 01:59:36 2014 -0700
summary:     Bug 716647 - Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them. (r=jandem)

This iteration took 208.183 seconds to run.

Comment 3

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

Shu-yu, is bug 716647 a possible regressor?

Comment 4

[Shu-yu Guo [:shu]]

Yep, part 1 is most likely the culprit. Sorry bug 716647 is setting off the fuzzer left and right, should've asked for fuzzing beforehand. :/

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Don't optimize out argument slots from resume points.

Existing bug.

What's going on is that Ion is optimizing out unused argument slots. If you run
a shell from rev d34458e80bcb~1, you can see that the second call to |inline|
returns |undefined| with --ion-eager, but |0| without --ion-eager. Differential
testing didn't hit this, I suppose.

Function.arguments basically means a function's argument slots can never be
optimized out of resume points. I hate this patch but I think our hands are tied here.

Jan, is |uses->index()| the right way to get the frame slot here?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8415756 [details] [diff] [review]
Don't optimize out argument slots from resume points.

Nicolas may be a better reviewer for this patch.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8415756 [details] [diff] [review]
Don't optimize out argument slots from resume points.

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

Whoa, I am surprised that such bugs hit us only now.

We should be able to make a test case for such problem where we return the value of arguments[0].  This should highlight a differential behavior prior to Bug 716647 as we used to replace the value by undefined instead of a magic value.

::: js/src/jit/IonAnalysis.cpp
@@ +139,5 @@
> +                // non-strict scripts, so we can't optimize out any arguments.
> +                if (!block->info().script()->strict()) {
> +                    uint32_t slot = uses->index();
> +                    uint32_t firstArgSlot = block->info().firstArgSlot();
> +                    if (firstArgSlot <= slot && slot - firstArgSlot < block->info().nargs()) {

nit: CompileInfo &info = block()->info();

Note, that this is safe to use the block info instead of the mrp->block()->info() because of the previous check which bails at doing such optimization if this resume point is part of a different block.  Thus there is no issue with arguments of the outermost scripts.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Comment on attachment 8415756 [details] [diff] [review]
> Don't optimize out argument slots from resume points.
> 
> Review of attachment 8415756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Whoa, I am surprised that such bugs hit us only now.
> 
> We should be able to make a test case for such problem where we return the
> value of arguments[0].  This should highlight a differential behavior prior
> to Bug 716647 as we used to replace the value by undefined instead of a
> magic value.

Sure, will do.

> 
> ::: js/src/jit/IonAnalysis.cpp
> @@ +139,5 @@
> > +                // non-strict scripts, so we can't optimize out any arguments.
> > +                if (!block->info().script()->strict()) {
> > +                    uint32_t slot = uses->index();
> > +                    uint32_t firstArgSlot = block->info().firstArgSlot();
> > +                    if (firstArgSlot <= slot && slot - firstArgSlot < block->info().nargs()) {
> 
> nit: CompileInfo &info = block()->info();
> 
> Note, that this is safe to use the block info instead of the
> mrp->block()->info() because of the previous check which bails at doing such
> optimization if this resume point is part of a different block.  Thus there
> is no issue with arguments of the outermost scripts.

Good to know.

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f0167547ce

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08f0167547ce