629974 - JM: Crash [@ js::gc::MarkId]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

for (a in [new String])
function b(foo) {
  foo.y = Math
  foo.y = function() {}
  Object.defineProperty(foo, "y", ({ set: Object }))
  gc()
  delete foo.y
}
for each(z in [new Boolean, Boolean]) {
  b(z)
}

crashes js opt shell on TM changeset 59d84fe951ba with -m at js::gc::MarkId but does not seem to have any undesired outcome in debug shell.

s-s and assuming [sg:critical?] because this involves gc.

===

(gdb) bt
#0  0x00131ba7 in js::gc::MarkId ()
#1  0x001305c1 in js::Shape::trace ()
#2  0x000bc42f in js_TraceObject ()
#3  0x000bcaf7 in js_TraceObject ()
#4  0x0002a1a5 in array_trace ()
#5  0x000a4fa3 in js::NativeIterator::mark ()
#6  0x000bc77f in js_TraceObject ()
#7  0x0007514d in js::gc::MarkChildren ()
#8  0x000786e7 in js::MarkWordConservatively ()
#9  0x00078c79 in js::MarkStackRangeConservatively ()
#10 0x00034a38 in js::StackSpace::mark ()
#11 0x0007ce16 in js::MarkRuntime ()
#12 0x0007e4cd in GCUntilDone ()
#13 0x0007eafe in js_GC ()
#14 0x00012de2 in JS_GC ()
#15 0x000068b9 in GC ()
#16 0x00245346 in CallCompiler::generateNativeStub ()
#17 0x0024392b in js::mjit::ic::NativeCall ()
#18 0x003620a2 in ?? ()
#19 0x001ec3ea in js::mjit::JaegerShot ()
#20 0x0009cecf in js::Execute ()
#21 0x000185a8 in JS_ExecuteScript ()
#22 0x00005e54 in Process ()
#23 0x0000a832 in Shell ()
#24 0x0000adcf in main ()
(gdb) x/i $eip
0x131ba7 <_ZN2js2gcL6MarkIdEP8JSTraceri+375>:   testb  $0x2,(%esi)
(gdb) x/b $esi
0x0:    Cannot access memory at address 0x0

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   59243:9aa8c290f633
user:        Luke Wagner
date:        Tue Nov 30 18:17:46 2010 -0800
summary:     Bug 614653 - avoid O(n^2) rope node marking (r=gwagner)

Comment 2

[Luke Wagner [:luke]]

I don't think this is caused by comment 1.  Rather, I think that test case depends on exact conditions to hit the crash and comment 1 perturbs things.

First, here is a further reduced test case that still crashes for me (in opt):

  foo = {}
  foo.y = 3;
  foo.y = function () {}
  Object.defineProperty(foo, "y", { set:function(){} })
  gc()
  delete foo.y
  gc();

What happens is that foo.y starts out as a data prop, then becomes an accessor prop.  On the first gc(), the slot that used to be occupied by the foo.y data prop isn't marked since, logically, it isn't needed.  This effect is achieved by this little bit in js_TraceObject:

    uint32 nslots = obj->numSlots();
    if (!obj->nativeEmpty() && obj->slotSpan() < nslots)
        nslots = obj->slotSpan();

On the second gc(), after foo.y is deleted, obj->nativeEmpty() is true so we mark all the objects slots (?!!) which, since they weren't marked by the first gc(), is now trash.

This doesn't happen in debug mode since, #ifdef DEBUG, JSObject::putProperty sets vacated slots to a safe undefined value (it seems like a more poisonous value could be used -- say ObjectValue((JSObject *)0xdead).

So what about that goofy obj->nativeEmpty() conjunct above?  If I

-    if (!obj->nativeEmpty() && obj->slotSpan() < nslots)
+    if (obj->slotSpan() < nslots)

the crash is indeed fixed (jit-tests pass too).  However, I hardly understand this code, so I am like a child who wanders into the middle of a movie and wants to know...

hg annotate shows a long trail of seemingly behavior-preserving changes to this line.  This one is interesting though:
  http://hg.mozilla.org/tracemonkey/diff/c65ec297710c/js/src/jsobj.c

Perhaps Brendan, Igor, or Jason could drop some logic here?

Comment 3

[Luke Wagner [:luke]]

Also, the way this crashed is an object getting its lastProp field clobbered with memory which, when interpreted as a shape, had a NULL-string jsid.  Crashing on NULL-string jsid is exactly what we were seeing for bug 615098.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: attempt 1

I can't repro this. But I'm sure it's real. I think there might be two problems:

 - the problem with 'empty' objects pointed out by Luke
 - the possibility of scanning through slots deleted in the middle.

Since I can't see this, Luke, could you try this patch for me, and see if it fixes these test cases? I think it probably won't, that your suggested change is need as well, but I want to know for sure.

Comment 5

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

(In reply to comment #4)
> I can't repro this. But I'm sure it's real.

Definitely still occurs on Mac 10.6 with TM changeset 9459c4b15890 32-bit opt shell with -m.

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #4)
From IRL discussion with Dave: I think the fix is simply to take out the !obj->nativeEmpty() conjunct as comment 2 proposes.  Dave and I looked at how empty shapes are created and it seems that their slot span includes a class's reserved slots so that there is no need to special case !obj->nativeEmpty() (we're guessing this is a historical artifact of earlier designs of empty shapes).

Comment 7

[Brendan Eich [:brendan]]

Sorry, got behind on bugmail. This is a historical artifact, otherwise known as left over crap I should have removed :-/.

It is guaranteed that empty shape slot span covers class reserved slots, I had to work hard on that to remove scopes.

Thanks for fixing.

/be

Comment 8

[Luke Wagner [:luke]]

Attachment: patch

Comment 9

[David Mandelin [:dmandelin]]

This patch is wanted for beta 11, so please land today (Feb 1), and land to both tm and m-c.

Comment 10

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/c97ba1410e59
http://hg.mozilla.org/mozilla-central/rev/6de40288cc33

Comment 11

[Tony Chung [:tchung]]

Gary, can you verify the fix please?  thanks.

Comment 12

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

Verified on rev a1a8cd4accba

Comment 13

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

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