Closed
Bug 360612
Opened 18 years ago
Closed 18 years ago
GC can leave GSN cache pointing to destroyed script
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: igor, Assigned: brendan)
References
Details
(Keywords: fixed1.8.1.1)
Attachments
(1 file, 4 obsolete files)
15.64 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
[ This is trunk-only regression and spin-off of bug 347306 comment 20]. The patch for bug 347306 contains: > #ifdef JS_THREADSAFE > /* > * Set all thread local freelists to NULL. We may visit a thread's > * freelist more than once. To avoid redundant clearing we unroll the > * current thread's step. >+ * >+ * Also, in case a JSScript wrapped in an object was finalized, we clear >+ * the acx->gsnCache.script pointer and finish the cache's hashtable. > */ > memset(cx->thread->gcFreeLists, 0, sizeof cx->thread->gcFreeLists); > iter = NULL; > while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) { > if (!acx->thread || acx->thread == cx->thread) > continue; > memset(acx->thread->gcFreeLists, 0, sizeof acx->thread->gcFreeLists); >+ JS_CLEAR_GSN_CACHE(acx); > } > #endif If there are more JSContext associated with the current thread besides the current one with GSN cache, then the loop will leave the caches uncleared. Thus they may point to a destroyed JSScript instance.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
Need this to land the patch for bug 357306 on the 1.8 branch, and for js1.7src. /be
Reporter | ||
Comment 3•18 years ago
|
||
Comment on attachment 246203 [details] [diff] [review] fix >--- jsgc.c 27 Oct 2006 19:38:33 -0000 3.180 >+++ jsgc.c 21 Nov 2006 21:50:36 -0000 >@@ -2819,19 +2819,23 @@ js_GC(JSContext *cx, JSGCInvocationKind > #endif > > #ifdef JS_THREADSAFE > /* > * Set all thread local freelists to NULL. We may visit a thread's > * freelist more than once. To avoid redundant clearing we unroll the > * current thread's step. > * >- * Also, in case a JSScript wrapped in an object was finalized, we clear >- * the acx->gsnCache.script pointer and finish the cache's hashtable. >+ * Also, in case a JSScript wrapped within an object was finalized, we >+ * null acx->thread->gsnCache.script and finish the cache's hashtable. >+ * Note that js_DestroyScript, called from script_finalize, will have >+ * already cleared cx->thread->gsnCache above during finalization, so we >+ * don't have to here. > */ >+ JS_ASSERT(!cx->thread->gsnCache.script); > memset(cx->thread->gcFreeLists, 0, sizeof cx->thread->gcFreeLists); > iter = NULL; > while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) { > if (!acx->thread || acx->thread == cx->thread) > continue; > memset(acx->thread->gcFreeLists, 0, sizeof acx->thread->gcFreeLists); > JS_CLEAR_GSN_CACHE(acx); > } This does not cover single-threaded case. For a minimal patch why not just move the cache nullifier code into the general context marking loop in js_GC and just do it there when acx != cx ?
Attachment #246203 -
Flags: review?(igor.bukanov) → review-
Assignee | ||
Comment 4•18 years ago
|
||
Good idea -- also, the patch for bug 361451 provokes the question: why sweep atoms before objects, rather than after (as property tree nodes and script filenames are done)? /be
Assignee | ||
Comment 5•18 years ago
|
||
mrbkap, please feel free to review too. /be
Attachment #246338 -
Flags: review?(igor.bukanov)
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 246338 [details] [diff] [review] fix, v2 Whoops, wrong bug. /be
Attachment #246338 -
Attachment is obsolete: true
Attachment #246338 -
Flags: review?(igor.bukanov)
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #246203 -
Attachment is obsolete: true
Attachment #246342 -
Flags: review?(igor.bukanov)
Reporter | ||
Comment 8•18 years ago
|
||
Comment on attachment 246342 [details] [diff] [review] fix, v2 >+#else >+ /* >+ * The thread-unsafe case just has to clear each context's GSN cache, >+ * except for cx's of course (see previous comment). >+ */ >+ while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) { >+ if (acx == cx) >+ continue; >+ JS_CLEAR_GSN_CACHE(acx); >+ } > #endif This is very different behaviour between single and multi threaded cases. Why not to store the cache in the runtime when JS_THREADSAFE is not defined?
Assignee | ||
Comment 9•18 years ago
|
||
Hope the single-threaded fill/hit-repeatedly-on-one-thread pattern is the common case, with no cache thrashing among contexts. /be
Attachment #246342 -
Attachment is obsolete: true
Attachment #246417 -
Flags: review?(igor.bukanov)
Attachment #246342 -
Flags: review?(igor.bukanov)
Reporter | ||
Updated•18 years ago
|
Attachment #246417 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 10•18 years ago
|
||
Fixed on trunk: Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.127; previous revision: 3.126 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.184; previous revision: 3.183 done Checking in jsscript.c; /cvsroot/mozilla/js/src/jsscript.c,v <-- jsscript.c new revision: 3.122; previous revision: 3.121 done /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 11•18 years ago
|
||
This patch seems to have caused the current tinderbox orange.
Comment 12•18 years ago
|
||
The stack to the crash and assertion failures that I see locally with that patch applied are at http://pastebin.mozilla.org/1539.
Assignee | ||
Comment 13•18 years ago
|
||
Backed out, completely botched the initialization of JSThread.gsnCache. Revised patch soon. /be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•18 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 14•18 years ago
|
||
Sorry for the blunder -- this works in my Firefox build. /be
Attachment #246417 -
Attachment is obsolete: true
Attachment #246451 -
Flags: review?(igor.bukanov)
Reporter | ||
Updated•18 years ago
|
Attachment #246451 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 15•18 years ago
|
||
Checking in jsapi.c; /cvsroot/mozilla/js/src/jsapi.c,v <-- jsapi.c new revision: 3.290; previous revision: 3.289 done Checking in jscntxt.c; /cvsroot/mozilla/js/src/jscntxt.c,v <-- jscntxt.c new revision: 3.94; previous revision: 3.93 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.129; previous revision: 3.128 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.187; previous revision: 3.186 done Checking in jsscript.c; /cvsroot/mozilla/js/src/jsscript.c,v <-- jsscript.c new revision: 3.124; previous revision: 3.123 done /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: in-testsuite-
Updated•18 years ago
|
Comment 16•18 years ago
|
||
blocking 1.8.1.1 because it's a regression from a 1.8.1.1-blocking bug (for verification testing purposes).
Updated•18 years ago
|
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1.1
You need to log in
before you can comment on or make changes to this bug.
Description
•