TM: "Assertion failure: hasInt32Repr(*vp),"

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: gkw, Assigned: luke)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][fixed-in-tracemonkey])

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
function h() {
  (function() {
    x = x % 4
  })()
}
x = 0
h()
for (a = 0; a < 12; ++a) {
  if (a == 7) {
    if (x) {} else {
      __defineSetter__("x", Object.defineProperties)
    }
  }
}

asserts js debug shell on TM changeset 72cb2f4a893c with -j at Assertion failure: hasInt32Repr(*vp), but does not seem to crash on opt shells.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54473:8deef18e795a
parent:      54471:f1912fe1996f
user:        Brendan Eich
date:        Tue Sep 21 00:04:25 2010 -0700
summary:     Fix slot leak that leads to allocSlot assert botch (597945, r=jorendorff).
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?

Updated

8 years ago
Assignee: general → pbiggar
blocking2.0: ? → final+
Whiteboard: softblocker

Comment 1

8 years ago
Further reduced:

x = 0
for (a = 0; a < 12; ++a) {
  if (a == 7) {
    if (!x) {
      __defineSetter__("x", Object.defineProperties)
    }
  }
}

If I make the condition !y (after setting y to 0), it stops asserting.
So this should be caught by the AbortRecordingIfUnexpectedGlobalWrite in js_DefineNativeProperty.  However, this test is being skipped because
  obj->containsSlot(shape->slot)
is false.

I'm not very familiar with property setting.  Jason or Waldo: perhaps you know of a better place to stick AbortRecordingIfUnexpectedGlobalWrite?
The setter has no slot (shape->slot is SHAPE_INVALID_SLOT -- can you confirm?), so that test won't help. But we're replacing a shape for 'x' in the global object that did have a slot.

Seems to me that AbortRecordingIfUnexpectedGlobalWrite needs to happen unconditionally, but that long-named static inline from jstracer.h is a new one on me. At the top of js_DefineNativeProperty is a LeaveTraceIfGlobalObject. AbortRecording and LeaveTrace events must line up. Can we fold the latter into the former?

/be
If adding a setter that does not replace a slotful prop then there's no hazard, so we could be more precise and optimize better, based on checking the pre-shape if any (it's looked up here but not saved):

        added = !obj->nativeContains(id);
        uint32 oldShape = obj->shape();
 . . .

/be
(In reply to comment #3)
> but that long-named static inline from jstracer.h is a new one on me.

Its only used in 3 places, so the long name seemed justified.  Also, I bet you knew immediately what it did ;-).

> AbortRecording and LeaveTrace events must line up. Can we fold the latter into
> the former?

The complication is that we need a slot to pass to AbortRecordingIfUnexpectedGlobalWrite, so we need to delay it until when we know the slot that is being changed.
(In reply to comment #5)
> (In reply to comment #3)
> > but that long-named static inline from jstracer.h is a new one on me.
> 
> Its only used in 3 places, so the long name seemed justified.  Also, I bet you
> knew immediately what it did ;-).

Hey, I didn't write "overlong" :-P. It's fine, but sometimes long names and comments are a warning sign of insufficient synthesis or minimization, or just unavoidable complexity (like this case).

> > AbortRecording and LeaveTrace events must line up. Can we fold the latter into
> > the former?
> 
> The complication is that we need a slot to pass to
> AbortRecordingIfUnexpectedGlobalWrite, so we need to delay it until when we
> know the slot that is being changed.

So we need to know when we're redefining a slot-based property, whether to a new definition that's slotless, or just a new slotful data property. That says we must use nativeLookup and not just capture the boolean "added".

/be
Created attachment 508447 [details] [diff] [review]
patch

Like this?
Assignee: pbiggar → lw
Status: NEW → ASSIGNED
Attachment #508447 - Flags: review?(brendan)
Comment on attachment 508447 [details] [diff] [review]
patch

Nice. "existingShape" is a fine name, but right below is "uint32 oldShape" which makes me sad for not renaming shape-ids to something like (here) oldShapeId. r=me in any case.

/be
Attachment #508447 - Flags: review?(brendan) → review+
Hmm, I see.  I went with "existing" because the Shape possibly pointed to by existingShape isn't itself being replaced or updated, so the old/new distinction didn't quite make sense.  I guess this is what you mean by 'shape' vs. 'shape-id' (the latter referring to the uint32 associated with an object, right?).  Perhaps just s/oldShape/oldShapeId/ ?
Don't worry about it -- something we can clean up later, not this patch.

/be
http://hg.mozilla.org/tracemonkey/rev/09a7b1270012
Whiteboard: softblocker → [softblocker][fixed-in-tracemonkey]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug628564.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.