501690 - Property cache assertion when 3 objects along a prototype chain have the same shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

function B(){}
B.prototype = {x: 123};

function D(){}
D.prototype = new B;
D.prototype.x = 1;    // [1] shapeOf(B.prototype) == shapeOf(D.prototype)

arr = [new D, new D, new D, D.prototype];  // [2] all the same shape
for (var i = 0; i < 4; i++)
    assertEq(arr[i].x, 1);  // same kshape [2], same vshape [1]

The first time through the loop, arr[i].x gets a property cache entry with protoIndex==1.  We do not detect that this property cache entry is invalid for the last element of arr, which has an own property x.

With -j:
  ../crasher.js:12: TypeError: Assertion failed: got 123, expected 1
Without -j:
  Assertion failure: pobj == found, at ../jsinterp.cpp:2605
  Trace/breakpoint trap

Comment 1

[Jason Orendorff [:jorendorff]]

See bug 497789 comment 39 for my take on this bug.

Comment 2

[Samuel Sidler (old account; do not CC)]

Jason: Who's a good owner for this bug?

Comment 3

[Jason Orendorff [:jorendorff]]

I'll take it. There's a patch in bug 503080 that fixes this.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

There's no given rationale as to why this needs to block a stability release, so minusing. Feel free to renominate with rationale.

Comment 5

[Boris Zbarsky [:bzbarsky]]

To be clear, this can at the very least cause the wrong functions to be called during JS execution on trace.  Not sure whether it can also cause the jit code to do weird things like reading from (or worse writing to) slots on an object that doesn't have any, but I bet Jason can tell you that by now....

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

The patch in bug 503080 which fixes this is huge and not something that's been nominated for the branch; is there a branch-only patch we want here?

Based on the discussion so far, I don't think this blocks 1.9.2, but we'd take a minimal branch patch as the symptom sounds ... not good.

If someone from the JS team can help clarify the effects and importance of taking this bug on the branch, it would be appreciated!

Comment 7

[Samuel Sidler (old account; do not CC)]

(In reply to comment #6)
> Based on the discussion so far, I don't think this blocks 1.9.2, but we'd take
> a minimal branch patch as the symptom sounds ... not good.

s/1.9.2/1.9.1.2

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> The patch in bug 503080 which fixes this is huge

It's not huge, just big-boned. Seriously, it's not much bigger than it needs to be. Any spot-fix (if one exists) could easily have problems preventing it from landing, e.g. regressing Tsspider.

I think we should take the patch for bug 503080 once it has baked. We have good regression-blaming so we can tell if it caused something caught by testers of tm and m-c.

Any regression beyond that would require a 1.9.2/fx.next scale of release to find, presuming our automated tests didn't find it. But this bug is serious too. You have to trade away this known risk for small (I claim) unknown risk at some point.

/be

Comment 9

[Damon Sicore (:damons)]

P1 as this is scary.  Touching the property cache in an invasive way.

Comment 10

[Robert Sayre]

this should be fixed by bug 503080, fixed on 1.9.2 since it was landed before the branch