Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: gkw, Assigned: luke)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey][fx4-fixed-bugday], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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
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)
Blocks: 614653
blocking2.0: --- → ?
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?
No longer blocks: 614653
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.
Blocks: 615098
blocking2.0: ? → betaN+
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?][hardblocker]
Posted patch attempt 1 (obsolete) — Splinter Review
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.
(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.
(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).
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
Assignee: general → lw
Posted patch patchSplinter Review
Attachment #508587 - Attachment is obsolete: true
Attachment #508841 - Flags: review?(brendan)
Whiteboard: [ccbr][sg:critical?][hardblocker] → [ccbr][sg:critical?][hardblocker][has patch]
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch] → [ccbr][sg:critical?][hardblocker][has patch][b11]
Attachment #508841 - Flags: review?(brendan) → review+
This patch is wanted for beta 11, so please land today (Feb 1), and land to both tm and m-c.
http://hg.mozilla.org/tracemonkey/rev/c97ba1410e59
http://hg.mozilla.org/mozilla-central/rev/6de40288cc33
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11] → [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey]
Gary, can you verify the fix please?  thanks.
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey] → [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey][fx4-fixed-bugday]
Verified on rev a1a8cd4accba
Status: RESOLVED → VERIFIED
Status: VERIFIED → RESOLVED
Closed: 9 years ago9 years ago
Crash Signature: [@ js::gc::MarkId]
Group: core-security
You need to log in before you can comment on or make changes to this bug.