458288 - TM: Heap corruption with gczeal, yield

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

$ ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j a5.js
js(40337) malloc: *** error for object 0x302f90: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Crash [@ szone_free] attempting to access 0xfffff804

$ valgrind ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j a5.js
Invalid write of size 4 at nanojit::Fragment::blacklist
Address was freed during GC js_FlushJITCache

Comment 1

[Jesse Ruderman]

Attachment: valgrind output

14 invalid reads, 2 invalid writes.

Comment 2

[Jesse Ruderman]

Still happens on tracemonkey branch after nanojit2 landing.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: proposed fix

If js_Interpret sees that the JIT cache was flushed, it sets a "safe delete" flag in addition to the deep abort flag, which causes it to not read from its fragment.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Whoops, ignore that erroneous "pendingFlush")

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 344384 [details] [diff] [review]
proposed fix

>diff -r 35c34996d80e js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Tue Oct 21 15:21:23 2008 -0700
>+++ b/js/src/jscntxt.h	Wed Oct 22 15:30:57 2008 -0700
>@@ -141,6 +141,8 @@ typedef struct JSTraceMonitor {
>     JSFragmentCacheEntry    fcache[JS_FRAGMENT_CACHE_SIZE];
>     jsval                   *recoveryDoublePool;
>     jsval                   *recoveryDoublePoolPtr;
>+    uint32                  cacheId;
>+    JSBool                  pendingFlush;

Deadwood as noted.

>+    uint32 traceMonCacheId = JS_TRACE_MONITOR(cx).cacheId;

Suggest jitCacheId as name (to match js_FlushJITCache).

>+        if (fragment->root == fragment

isRootFragment should be equivalent and faster to test?

>+    if (f == NULL) {

Nit: !f as noted.

>+    tm->cacheId++;

cacheId is a fine name, some slight pref for cacheGen (generation) to match other generation-number-based cache flushing in SpiderMonkey.

r=me with above addressed.

/be

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Pushed changeset: http://hg.mozilla.org/tracemonkey/rev/dcf3b1f789e8

Comment 7

[Andreas Gal :gal]

Attachment: improved patch

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 345187 [details] [diff] [review]
improved patch

Easy come, easy go! Srsly, do we have a test for the bug that was fixed by the jitCacheGen patch?

/be

Comment 9

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/fca419901097

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-458288.js

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed mozilla-central, tracemonkey

Comment 12

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.