323267 - bug in js_GC due to js_SweepScriptFilenames before finalizer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alexander Mohr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: SpiderMonkey v1.6

if you have a function linked to a script (fun->u.script), and that
function is about to be GC'd,  (~ line 1835 in jsgc.c) the script
filenames are freed due to the call to
js_SweepScriptFilenames.

However, on ~ line 1860 the finalizer is
called for that function object, which will then destroy the script,
and potentially pass a freed script filename pointer to the
destroyScriptHook. 

Reproducible: Always

Steps to Reproduce:
1. Register DestroyScriptHook (JS_SetDestroyScriptHook)
2. evaluate Script which causes "scriptlette"
i.e.:" /* foo */
       function foo() { }

3. call GC

Actual Results:  
Your DestroyScriptHook gets called with a GC'd filename pointer in the JSScript.

Expected Results:  
JSScript->filename should still be valid.

Comment 1

[Igor Bukanov]

Attachment: Fix

The patch moves script file name seeping after the main GC sweep loop so script file names are still available for a debug hook. Effectively it enforces the pattern for this particular case that in the graph of things to free the leaves should be freed after the rest of nodes.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 208416 [details] [diff] [review]
Fix

>Index: src/jsgc.c
>===================================================================
>--- src.orig/jsgc.c
>+++ src/jsgc.c
>@@ -1863,6 +1862,13 @@ restart:
>             }
>         }
>     }

Nit: blank line here per style guide (http://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style).

>+    /*
>+     * Sweep scrip file names after sweeping strings and functions in the

Nit: "scrip" typo.  Bonus if "filenames" is one word (it's in my hacker lexicon ;-).

>+     * generic loop above. In this way when interpreted function finalizer

Nit: "an interpreted function's finalizer" or "a scripted function's finalizer".

>+     * destroys script triggering a call to rt->destroyScriptHook, the hook
>+     * can still access script's filename.
>+     */
>+    js_SweepScriptFilenames(rt);

Fast action patching this, thanks Igor!

/be

Comment 3

[Igor Bukanov]

Attachment: Patch to commit

This a version to commit with nits addressed.

Comment 4

[Igor Bukanov]

I committed the patch.

Comment 5

[Igor Bukanov]

Attachment: C source of the test case.

To test the bug on Linux, place the source into js/src directory, compile js library using make -f Makefile.ref, compile the source using:

gcc test323267.c -DXP_UNIX -DDEBUG -I. -ILinux_All_DBG.OBJ -LLinux_All_DBG.OBJ -ljs -g -o test323267

and run it via

LD_LIBRARY_PATH=Linux_All_DBG.OBJ test323267

Without the patch applied it generates "Segmentation fault", with the patch it prints "SUCCEEDED".

Now it would be nice if it would be possible to extend the test suite with C sources to test for all those callback-only bugs.

Comment 6

[Igor Bukanov]

Attachment: C source of the test case v2

The previous version had wrong free() call.

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #5)
> Now it would be nice if it would be possible to extend the test suite with C
> sources to test for all those callback-only bugs.

I've been thinking about that and it is conceivable that we could do that. Let's mark requests that require compiling somehow. Maybe [compile-test?] in the status whiteboard?

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 208436 [details] [diff] [review]
Patch to commit

Another clean fix for the 1.8x branches.

/be

Comment 9

[Daphne H. Luong]

We ran into this core dump recently.  We are currently using version 1.5 RC6 of the SpiderMonkey code base and the version of jsgc.c we have is different from that posted in the patch.  Can we apply the patch by simply moving js_SweepScriptFilenames from where it is to right before the "Free phase" section?

If attaching the code between the 'Sweep phase' and 'Free phase' of the version of jsgc.c source I have is helpful, please let me know.  Thanks.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Can we apply the patch by simply moving
> js_SweepScriptFilenames from where it is to right before the "Free phase"
> section?

Sure, just move that js_SweepScriptFilenames call.

You guys will want to try out the forthcoming JS1.6 RC1 -- it should drop in and it has fixes for bugs that might be biting you.

/be

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 208436 [details] [diff] [review]
Patch to commit

Another double-whammy.

/be

Comment 12

[Daniel Veditz [:dveditz]]

Fix checked into 1.8 and 1.8.0 branches

Comment 13

[Dave Liebreich [:davel]]

bc, can you verify this fix?  If not, please reflag this as [tcn-dl] in the status whiteboard.

Comment 14

[Daniel Veditz [:dveditz]]

Just found I missed the 1.8 branch, checked in for real.
Trunk:    r3.111          (2006-01-14)
1.8.0.2:  r3.104.2.3.2.4  (2006-02-26)
1.8:      r3.104.2.7      (2006-05-05)