870200 - Assertion failure: (ptrBits & 0x7) == 0, at ./dist/include/js/Value.h:703 or Crash [@ GetValueType]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision e19d0885977c (run with --ion-eager):


function getter(a, i) {
  return a[i];
}
function foo(... a) {
  res = 0;
  for (var j = 0; j < 100 ; j++) {
    res += getter(a, j);
  }
}
var n = 100;
var a = Array(n);
var q = foo(a, n);

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

Pretty sure there's something going wrong in memory here, in a 32 bit build I don't get the assert but get the crash directly even in a debug build.

Comment 3

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   127209:ed51ae24fee2
user:        Kannan Vijayan
date:        Wed Jan 30 12:09:10 2013 -0500
summary:     Bug 805877 - Bailout from Ion to Baseline. r=jandem

This iteration took 128.277 seconds to run.

Comment 4

[Andrew McCreight [:mccr8]]

This assertion looks bad, so I'm marking it sec-high.  Could you maybe look at this djvj?

Comment 5

[Kannan Vijayan [:djvj]]

What's happening here is that Ion is compiling |getter| (it compiles once and then invalidates, and then recompiles), but the LoadElement operation it uses for the |a[i]| expression is typed as Object during the second compile.  However, |a| in that context is actually the composed array: [Array(n), n] captured by the rest-arg call to |foo| (n=100 in this case).  When |j=1|, the LoadElement is loading the second value in the array (the integer n) as if it were an object.

Tracked it down some more.  The TypeObject being assigned the created rest-argument array is happening after the array is created:

111578: BEGIN_CASE(JSOP_REST)
111578: {
111578:     RootedObject &rest = rootObject0;
111578:     rest = regs.fp()->createRestParameter(cx);
111578:     if (!rest)
111578:         goto error;
111578:     PUSH_COPY(ObjectValue(*rest));
120931:     if (!SetInitializerObjectType(cx, script, regs.pc, rest, GenericObject))
111578:         goto error;
120931:     rootType0 = GetTypeCallerInitObject(cx, JSProto_Array);
120931:     if (!rootType0)
120931:         goto error;
120931:     rest->setType(rootType0);
111578: }
111578: END_CASE(JSOP_REST)

Also weird,  |SetInitializerObjectType| is expected to initialize |rest|'s type.  But this code additionally gets another TypeObject via |GetTypeCallerInitObject| and resets rest's type again.

I don't know what the intent is here.

Terrence: The 'hg blame' for the lines with the TypeObject changes point to one of your pushes.  Can you explain the intent of the type code here?  Why do we re-set the type on |rest| when |SetInitializerObjectType| should have already done it?

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: Hacky fix

This patch "solves" the issue, but in a horrible way.  It just manually loops around the array after it's been created and the type set, and calls the type update routine for every value in the array.

Presumably there's a better way of doing this, and it probably involves changing |StackFrame::createRestParameter| to calculate the type, set it, and ensure that all value types are updated.

This is a good opportunity to clean up the JSOP_REST handling in the interpreter too, so I'll look in to that.

In the meantime, this patch makes the crash go away.

Comment 7

[Terrence Cole [:terrence]]

Good find! This looks like fallout from moving singleton selection into the allocator.

So, the problem we had before was that we would update an object to be a singleton /after/ allocating it; however, we need all singletons to start tenured because they can get baked into jit code. To solve this, I munged all sites that could be singletons to pass SingletonObject in this case.

When Benjamin added JSOP_REST, he (quite reasonably) copied the array initialization code directly from JSOP_NEWARRAY. In the case of JSOP_NEWARRAY, we may want to create the array as a singleton -- the UseNewTypeForInitializer call in that opcode checks this. For JSOP_REST, however, we probably never want this. Since I needed to change the code anyway, Brian had me update this site to never create the rest array as Singleton.

Given that this code is a horrid amalgam of two separate cargo cultings, I expect it is probably just wrong.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: Fix

Fix modifies |StackFrame::createRestParameter| to also do the TypeObject initialization for the created array.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 753328 [details] [diff] [review]
Fix

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

::: js/src/vm/Stack.cpp
@@ +244,5 @@
> +    obj->setType(type);
> +
> +    /* Ensure that values in the rest array are represented in the type of the array. */
> +    for (unsigned i = 0; i < nrest; i++)
> +        types::AddTypePropertyId(cx, obj, JSID_VOID, restvp[i]);

It might be simpler to just drop this block and the GetTypeCallerInitObject stuff above, so that the array retains its generic type and has unknown properties.  Barriers will be needed when reading from the array, but that's no worse than what we do for arguments accesses.

Comment 10

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> ::: js/src/vm/Stack.cpp
> @@ +244,5 @@
> > +    obj->setType(type);
> > +
> > +    /* Ensure that values in the rest array are represented in the type of the array. */
> > +    for (unsigned i = 0; i < nrest; i++)
> > +        types::AddTypePropertyId(cx, obj, JSID_VOID, restvp[i]);
> 
> It might be simpler to just drop this block and the GetTypeCallerInitObject
> stuff above, so that the array retains its generic type and has unknown
> properties.  Barriers will be needed when reading from the array, but that's
> no worse than what we do for arguments accesses.

I like the idea of leaving them in.  It's reasonable to guess that at least in some cases, we will see stable type patterns for the rest arguments arrays.  Would be nice to catch those.

Comment 11

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

Kannan has a few patches here, so assigning as-is.

Comment 12

[Kannan Vijayan [:djvj]]

I was going to land this earlier today but CLOSED TREE :(

Comment 13

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a17bb92b2d52

Comment 14

[Kannan Vijayan [:djvj]]

Backed out for bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/8de86a614249

Fixed and re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/8de86a614249

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Shouldn't this be backported since a Jan 30 regression affects Firefox 21 and higher?

Comment 18

[Alex Keybl [:akeybl]]

(In reply to Al Billings [:abillings] from comment #17)
> Shouldn't this be backported since a Jan 30 regression affects Firefox 21
> and higher?

Unless this is highly obvious/exploitable, or super low risk, this will be a wontfix for FF22.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift nomination?

Comment 20

[Kannan Vijayan [:djvj]]

Comment on attachment 753328 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 706885

User impact if declined: 
Potentially exploitable error.

Testing completed (on m-c, etc.):
Green on m-c for about a month.
 
Risk to taking this patch (and alternatives if risky):
Low risk.

String or IDL/UUID changes made by this patch:
N/A

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f505ea8cb1c2