As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 360612 - GC can leave GSN cache pointing to destroyed script
: GC can leave GSN cache pointing to destroyed script
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 347306 js1.7src
  Show dependency treegraph
 
Reported: 2006-11-13 15:16 PST by Igor Bukanov
Modified: 2006-11-27 17:47 PST (History)
4 users (show)
dveditz: blocking1.8.1.1+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (9.32 KB, patch)
2006-11-21 13:53 PST, Brendan Eich [:brendan]
igor: review-
Details | Diff | Splinter Review
fix, v2 (6.72 KB, patch)
2006-11-22 13:52 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
fix, v2 (11.52 KB, patch)
2006-11-22 14:11 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
fix, v3 (12.15 KB, patch)
2006-11-23 10:56 PST, Brendan Eich [:brendan]
igor: review+
Details | Diff | Splinter Review
fix, v4 (15.64 KB, patch)
2006-11-23 22:47 PST, Brendan Eich [:brendan]
igor: review+
Details | Diff | Splinter Review

Description User image Igor Bukanov 2006-11-13 15:16:31 PST
[ 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.
Comment 1 User image Brendan Eich [:brendan] 2006-11-21 13:53:14 PST
Created attachment 246203 [details] [diff] [review]
fix
Comment 2 User image Brendan Eich [:brendan] 2006-11-21 13:55:21 PST
Need this to land the patch for bug 357306 on the 1.8 branch, and for js1.7src.

/be
Comment 3 User image Igor Bukanov 2006-11-21 19:09:55 PST
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 ?
Comment 4 User image Brendan Eich [:brendan] 2006-11-21 19:45:55 PST
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
Comment 5 User image Brendan Eich [:brendan] 2006-11-22 13:52:01 PST
Created attachment 246338 [details] [diff] [review]
fix, v2

mrbkap, please feel free to review too.

/be
Comment 6 User image Brendan Eich [:brendan] 2006-11-22 13:55:27 PST
Comment on attachment 246338 [details] [diff] [review]
fix, v2

Whoops, wrong bug.

/be
Comment 7 User image Brendan Eich [:brendan] 2006-11-22 14:11:08 PST
Created attachment 246342 [details] [diff] [review]
fix, v2
Comment 8 User image Igor Bukanov 2006-11-22 17:36:50 PST
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?
Comment 9 User image Brendan Eich [:brendan] 2006-11-23 10:56:54 PST
Created attachment 246417 [details] [diff] [review]
fix, v3

Hope the single-threaded fill/hit-repeatedly-on-one-thread pattern is the common case, with no cache thrashing among contexts.

/be
Comment 10 User image Brendan Eich [:brendan] 2006-11-23 19:03:49 PST
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
Comment 11 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-23 20:07:11 PST
This patch seems to have caused the current tinderbox orange.
Comment 12 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-23 20:21:03 PST
The stack to the crash and assertion failures that I see locally with that patch applied are at http://pastebin.mozilla.org/1539.
Comment 13 User image Brendan Eich [:brendan] 2006-11-23 21:14:07 PST
Backed out, completely botched the initialization of JSThread.gsnCache.  Revised patch soon.

/be
Comment 14 User image Brendan Eich [:brendan] 2006-11-23 22:47:07 PST
Created attachment 246451 [details] [diff] [review]
fix, v4

Sorry for the blunder -- this works in my Firefox build.

/be
Comment 15 User image Brendan Eich [:brendan] 2006-11-24 18:11:54 PST
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
Comment 16 User image Daniel Veditz [:dveditz] 2006-11-27 16:34:12 PST
blocking 1.8.1.1 because it's a regression from a 1.8.1.1-blocking bug (for verification testing purposes).

Note You need to log in before you can comment on or make changes to this bug.