Closed Bug 870200 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 - wontfix
firefox23 + fixed
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: decoder, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main23+])

Crash Data

Attachments

(2 files, 1 obsolete file)

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);
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.
Crash Signature: [@ GetValueType]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
This assertion looks bad, so I'm marking it sec-high.  Could you maybe look at this djvj?
Flags: needinfo?(kvijayan)
Keywords: sec-high
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?
Flags: needinfo?(kvijayan) → needinfo?(terrence)
Attached patch Hacky fix (obsolete) — Splinter Review
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.
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.
Flags: needinfo?(terrence)
Attached patch FixSplinter Review
Fix modifies |StackFrame::createRestParameter| to also do the TypeObject initialization for the created array.
Attachment #752990 - Attachment is obsolete: true
Attachment #753328 - Flags: review?(bhackett1024)
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.
Attachment #753328 - Flags: review?(bhackett1024) → review+
(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.
Kannan has a few patches here, so assigning as-is.
Assignee: general → kvijayan
Status: NEW → ASSIGNED
I was going to land this earlier today but CLOSED TREE :(
https://hg.mozilla.org/mozilla-central/rev/8de86a614249
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Shouldn't this be backported since a Jan 30 regression affects Firefox 21 and higher?
(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.
Flags: in-testsuite?
Uplift nomination?
Flags: needinfo?(kvijayan)
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
Attachment #753328 - Flags: approval-mozilla-beta?
Attachment #753328 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(kvijayan)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.