Closed Bug 323267 Opened 15 years ago Closed 15 years ago

bug in js_GC due to js_SweepScriptFilenames before finalizer

Categories

(Core :: JavaScript Engine, defect)

x86
Windows Server 2003
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: amohr, Assigned: igor)

Details

(Keywords: fixed1.8.0.2, fixed1.8.1, Whiteboard: [rft-dl])

Attachments

(3 files, 1 obsolete file)

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.
Attached patch FixSplinter Review
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.
Assignee: general → igor.bukanov
Status: UNCONFIRMED → ASSIGNED
Attachment #208416 - Flags: review?(brendan)
Flags: blocking1.8.0.2+
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
Attachment #208416 - Flags: review?(brendan) → review+
Attached patch Patch to commitSplinter Review
This a version to commit with nits addressed.
I committed the patch.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attached file C source of the test case. (obsolete) —
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.
The previous version had wrong free() call.
Attachment #208437 - Attachment is obsolete: true
(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 on attachment 208436 [details] [diff] [review]
Patch to commit

Another clean fix for the 1.8x branches.

/be
Attachment #208436 - Flags: review+
Attachment #208436 - Flags: approval1.8.1?
Attachment #208436 - Flags: approval1.8.0.2?
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.
(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
Attachment #208436 - Flags: approval1.8.1? → branch-1.8.1?(brendan)
Comment on attachment 208436 [details] [diff] [review]
Patch to commit

Another double-whammy.

/be
Attachment #208436 - Flags: branch-1.8.1?(brendan)
Attachment #208436 - Flags: branch-1.8.1+
Attachment #208436 - Flags: approval1.8.0.2?
Attachment #208436 - Flags: approval1.8.0.2+
Flags: testcase-
Fix checked into 1.8 and 1.8.0 branches
bc, can you verify this fix?  If not, please reflag this as [tcn-dl] in the status whiteboard.
Whiteboard: [rft-dl]
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)
You need to log in before you can comment on or make changes to this bug.