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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.1.1 -

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 -, status1.9.1 wanted)

Details

(Whiteboard: fixed-in-tracemonkey [fixed by bug 503080])

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
See bug 497789 comment 39 for my take on this bug.
Flags: blocking1.9.2?
Flags: blocking1.9.1.1?
Jason: Who's a good owner for this bug?
(Assignee)

Comment 3

9 years ago
I'll take it. There's a patch in bug 503080 that fixes this.
Assignee: general → jorendorff
Depends on: 503080
blocking1.9.1: --- → ?
Flags: blocking1.9.1.1? → blocking1.9.1.1-
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-tracemonkey
There's no given rationale as to why this needs to block a stability release, so minusing. Feel free to renominate with rationale.
blocking1.9.1: ? → -
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....
blocking1.9.1: - → ?
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!
blocking1.9.1: ? → -
status1.9.1: --- → wanted
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [fixed by bug 503080]
(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
(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

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+
P1 as this is scary.  Touching the property cache in an invasive way.
Priority: -- → P1

Comment 10

9 years ago
this should be fixed by bug 503080, fixed on 1.9.2 since it was landed before the branch
status1.9.2: --- → beta1-fixed

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.