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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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).

Comment 1

[Paul Biggar]

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.

Comment 2

[Luke Wagner [:luke]]

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?

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Luke Wagner [:luke]]

(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.

Comment 6

[Brendan Eich [:brendan]]

(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

Comment 7

[Luke Wagner [:luke]]

Attachment: patch

Like this?

Comment 8

[Brendan Eich [: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

Comment 9

[Luke Wagner [:luke]]

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/ ?

Comment 10

[Brendan Eich [:brendan]]

Don't worry about it -- something we can clean up later, not this patch.

/be

Comment 11

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/09a7b1270012

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/09a7b1270012

Comment 13

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug628564.js.