Closed Bug 345772 Opened 14 years ago Closed 14 years ago

GC hazard in cloning block chains

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: crash, fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

I turned on WAY_TOO_MUCH_GC the other day and ran Nanto's testcase and crashed. I finally got around to debugging it today (I'll attach the code that I used in a bit), but the problem is that while we're cloning block objects, we don't protect the newborn block objects...

In other words, we allocate the block object here:
js_NewGCThing (/home/mrbkap/mozbuild/mozilla/js/src/jsgc.c:1277)
js_NewObject (/home/mrbkap/mozbuild/mozilla/js/src/jsobj.c:2174)
js_CloneBlockObject (/home/mrbkap/mozbuild/mozilla/js/src/jsobj.c:1902)
CloneBlockChain (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:486)
CloneBlockChain (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:482)
js_GetScopeChain (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:514)
js_Interpret (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:5058)

And collect it here:
js_GC (/home/mrbkap/mozbuild/mozilla/js/src/jsgc.c:2725)
js_NewGCThing (/home/mrbkap/mozbuild/mozilla/js/src/jsgc.c:1129)
AllocSlots (/home/mrbkap/mozbuild/mozilla/js/src/jsobj.c:2072)
js_NewObject (/home/mrbkap/mozbuild/mozilla/js/src/jsobj.c:2226)
js_CloneBlockObject (/home/mrbkap/mozbuild/mozilla/js/src/jsobj.c:1902)
CloneBlockChain (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:486)
js_GetScopeChain (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:514)
js_Interpret (/home/mrbkap/mozbuild/mozilla/js/src/jsinterp.c:5058)
I meant Nanto's testcase from bug 345542.
Priority: -- → P1
Attached patch Debug codeSplinter Review
This code simply dumps every GC thing allocation and finalization to stderr... It's a lot like dbaron's patch in bug 307312, except that it doesn't depend on nsTraceRefcnt (and so I didn't bother tracking addrefs and releases).
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attached patch Fix (obsolete) — Splinter Review
Local root scopes come to the rescue!
Attachment #230492 - Flags: review?(shaver)
Attachment #230492 - Flags: review?(brendan)
Comment on attachment 230492 [details] [diff] [review]
Fix

Couldn't we just use fp->scopeChain to protect each cloned parent (chain) as we go?  Alterna-patch (with some bulk from code reloation -- PutBlockObjects should not have gone in between CloneBlockChain and js_GetScopeChain!) in a sec.

/be
Attachment #230492 - Flags: review?(brendan) → review-
Thanks for finding this.  I shoulda seen it!  It should not require local root scopes to fix.

/be
Attachment #230509 - Flags: review?(mrbkap)
Comment on attachment 230509 [details] [diff] [review]
alterna-fix, smaller ignoring PutBlockObjects relocation
Attachment #230509 - Flags: review?(mrbkap) → review+
Attachment #230492 - Attachment is obsolete: true
Attachment #230492 - Flags: review?(shaver)
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: js1.7
Flags: blocking1.8.1?
Attachment #230509 - Flags: approval1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 230509 [details] [diff] [review]
alterna-fix, smaller ignoring PutBlockObjects relocation

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #230509 - Flags: approval1.8.1? → approval1.8.1+
Is this relevant for the 1.8.0 branch?
Flags: blocking1.8.0.6?
(In reply to comment #9)
> Is this relevant for the 1.8.0 branch?

No -- block scope (let) is new in JS1.7.

I'll let mrbkap land this.

/be
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1
Flags: in-testsuite-
Not needed in 1.8.0 per comment 10
Flags: blocking1.8.0.7? → blocking1.8.0.7-
You need to log in before you can comment on or make changes to this bug.