Last Comment Bug 323267 - bug in js_GC due to js_SweepScriptFilenames before finalizer
: bug in js_GC due to js_SweepScriptFilenames before finalizer
Status: RESOLVED FIXED
[rft-dl]
: fixed1.8.0.2, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows Server 2003
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-12 23:24 PST by Alexander Mohr
Modified: 2006-05-05 13:35 PDT (History)
6 users (show)
brendan: blocking1.8.0.2+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (820 bytes, patch)
2006-01-13 15:09 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Patch to commit (875 bytes, patch)
2006-01-13 18:26 PST, Igor Bukanov
brendan: review+
brendan: approval‑branch‑1.8.1+
brendan: approval1.8.0.2+
Details | Diff | Splinter Review
C source of the test case. (1.83 KB, text/plain)
2006-01-13 18:38 PST, Igor Bukanov
no flags Details
C source of the test case v2 (1.83 KB, text/plain)
2006-01-13 18:41 PST, Igor Bukanov
no flags Details

Description Alexander Mohr 2006-01-12 23:24:49 PST
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 2006-01-13 15:09:08 PST
Created attachment 208416 [details] [diff] [review]
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] 2006-01-13 15:28:57 PST
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 2006-01-13 18:26:29 PST
Created attachment 208436 [details] [diff] [review]
Patch to commit

This a version to commit with nits addressed.
Comment 4 Igor Bukanov 2006-01-13 18:31:31 PST
I committed the patch.
Comment 5 Igor Bukanov 2006-01-13 18:38:43 PST
Created attachment 208437 [details]
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 2006-01-13 18:41:43 PST
Created attachment 208441 [details]
C source of the test case v2

The previous version had wrong free() call.
Comment 7 Bob Clary [:bc:] 2006-01-13 18:48:19 PST
(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] 2006-01-13 20:49:27 PST
Comment on attachment 208436 [details] [diff] [review]
Patch to commit

Another clean fix for the 1.8x branches.

/be
Comment 9 Daphne H. Luong 2006-01-19 00:22:40 PST
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] 2006-01-20 10:42:19 PST
(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] 2006-01-31 12:45:07 PST
Comment on attachment 208436 [details] [diff] [review]
Patch to commit

Another double-whammy.

/be
Comment 12 Daniel Veditz [:dveditz] 2006-02-26 11:15:33 PST
Fix checked into 1.8 and 1.8.0 branches
Comment 13 Dave Liebreich [:davel] 2006-03-02 14:11:04 PST
bc, can you verify this fix?  If not, please reflag this as [tcn-dl] in the status whiteboard.
Comment 14 Daniel Veditz [:dveditz] 2006-05-05 13:35:26 PDT
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)

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