Closed
Bug 629974
Opened 14 years ago
Closed 14 years ago
JM: Crash [@ js::gc::MarkId]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: gkw, Assigned: luke)
References
Details
(Keywords: crash, regression, testcase, Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey][fx4-fixed-bugday])
Crash Data
Attachments
(1 file, 1 obsolete file)
1.40 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
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: --- → ?
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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
Updated•14 years ago
|
blocking2.0: ? → betaN+
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?][hardblocker]
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
(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.
Assignee | ||
Comment 6•14 years ago
|
||
(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•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee: general → lw
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #508587 -
Attachment is obsolete: true
Attachment #508841 -
Flags: review?(brendan)
Updated•14 years ago
|
Whiteboard: [ccbr][sg:critical?][hardblocker] → [ccbr][sg:critical?][hardblocker][has patch]
Updated•14 years ago
|
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch] → [ccbr][sg:critical?][hardblocker][has patch][b11]
Updated•14 years ago
|
Attachment #508841 -
Flags: review?(brendan) → review+
Comment 9•14 years ago
|
||
This patch is wanted for beta 11, so please land today (Feb 1), and land to both tm and m-c.
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/c97ba1410e59
http://hg.mozilla.org/mozilla-central/rev/6de40288cc33
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11] → [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey]
Comment 11•14 years ago
|
||
Gary, can you verify the fix please? thanks.
Updated•14 years ago
|
Whiteboard: [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey] → [ccbr][sg:critical?][hardblocker][has patch][b11][fixed-in-tracemonkey][fx4-fixed-bugday]
Comment 13•14 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/c97ba1410e59
Updated•14 years ago
|
Status: VERIFIED → RESOLVED
Closed: 14 years ago → 14 years ago
Updated•13 years ago
|
Crash Signature: [@ js::gc::MarkId]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•