425596 - Scriptable JSFunctions being finalized without their scripts being destroyed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Spent a couple hours of the last few days trying to figure out this Firebug-related crash.

I was seeing a JSDScript being hit in enumerateScripts for which its ->function had been collected (0xdadadada in some slots).  I instrumented js_FinalizeFunction, and it wasn't being called for the script in question.  Then, on a hunch, I added a finalizer to js_FunctionClass, and sure enough, I found that the JSFunction was being finalized via the hacked-in finalizer, but there was no call being made to js_DestroyScript, and therefore the JSDScript referencing that function wasn't being torn down.

The obvious idea of having that hacky finalizer call js_FinalizeFunction if FUN_IS_SCRIPTED resulted in some malloc errors, likely a double-free.  So maybe the JSScriptedFunction would have been collected on the next GC anyway, but we looked at a bad time?  Perhaps NULLing out fun->script from js_FinalizeFunction would suffice to protect us, though we'd need to defend DestroyLocalNames as well.  And if we have multiple JSFunctions referencing a JSScriptedFunction, then that won't work...

I bet igor can name this tune in 3 notes...

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So I tried a few things here, like nulling out stuff to make the ScriptedFunction safe to finalize twice, but then I realized that the engine was probably self-consistent, it was just that JSD didn't expect that the script could outlive its function.  (A reasonable belief on its part, given the API for the newScriptHook, IMO.)

So my new solution, which seems to work, is to add fun_finalize, and if FUN_IS_SCRIPTED(fun) then eagerly call js_CallScriptDestroyHook.  This is not architecturally pure, and I am almost now angry enough at jsd to start writing a test suite as a prelude to major cleanup.

In the interim, though, I will attach a patch that resolves this issue via the aforementioned eager script notification.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Notify debugger on function finalization as well

Haven't run this through mochi or suite yet, but since there's no JSD interaction in any of those cases, I'm boldly requesting review since it very much seems to fix the crash.  I'll do a test set before committing, for sure.

(For sure!)

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 312300 [details] [diff] [review]
Notify debugger on function finalization as well

If js_DestroyScript is not called, a great many things besides not notifying the debugger fail to happen. That's a bad bug. Why isn't the script being destroyed? If the function owns the script then it is obligated to do so, and the way to do it is via fun_finalize.

/be

Comment 4

[Igor Bukanov]

Comment on attachment 312300 [details] [diff] [review]
Notify debugger on function finalization as well

>Index: jsfun.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsfun.c,v
>retrieving revision 3.271
>diff -u -p -8 -r3.271 jsfun.c
>--- jsfun.c	23 Mar 2008 10:04:38 -0000	3.271
>+++ jsfun.c	28 Mar 2008 15:48:23 -0000
>@@ -1413,29 +1413,43 @@ fun_reserveSlots(JSContext *cx, JSObject
>      * slots of the function object is set.
>      */
>     sfun = FUN_TO_SCRIPTED(funobj);
>     return (sfun && sfun->script && sfun->script->regexpsOffset != 0)
>            ? JS_SCRIPT_REGEXPS(sfun->script)->length
>            : 0;
> }
> 
>+static void
>+fun_finalize(JSContext *cx, JSObject *obj)
>+{
>+    JSFunction *fun = OBJ_TO_FUNCTION(obj);
>+
>+    /*
>+     * Notify debugger early of script destruction, since the API implies
>+     * that the function and script have identical lifetime.  See bug
>+     * 425596.
>+     */
>+    if (FUN_IS_SCRIPTED(fun))
>+        js_CallDestroyScriptHook(cx, FUN_TO_SCRIPTED(fun)->script);
>+}

1. FUN_TO_SCRIPTED(fun) can be finalized before the obj. In fact it should be finalized before obj due to the way js_GC works. 

2. This will call js_CallDestroyScriptHook multiple times for cloned functions. Also the hook would still be called from js_FinalizeScriptedFunction.

Comment 5

[Igor Bukanov]

(In reply to comment #0)
> I instrumented
> js_FinalizeFunction, and it wasn't being called for the script in question. 

That must be the real bug. Does the debuger uses JSFunction* or JSObject* in that code to refer to functions?

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #4)
> 1. FUN_TO_SCRIPTED(fun) can be finalized before the obj. In fact it should be
> finalized before obj due to the way js_GC works. 

That means that sfunOrClass dangles between those finalizations?  I assure you that in this case it's not being finalized first, for the script I'm crashing on.

> 2. This will call js_CallDestroyScriptHook multiple times for cloned functions.
> Also the hook would still be called from js_FinalizeScriptedFunction.

Yes, it will call the hook multiple times; jsd defends itself against that nicely (and generally debuggers need to, because they can't be sure that they saw all scripts get created, so they might get mismatched removes).

What do you propose as an alternative?  What data should I gather?  I can definitely tell you that the function is being finalized before its sfun's script gets destroyed.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #5)
> (In reply to comment #0)
> > I instrumented
> > js_FinalizeFunction, and it wasn't being called for the script in question. 
> 
> That must be the real bug. Does the debuger uses JSFunction* or JSObject* in
> that code to refer to functions?

It uses JSFunction * everywhere to refer to functions.  And when I say "wasn't being called" I mean "wasn't called by the time I crashed in jsd" -- for all I know it would have been picked up on the next GC pass, or otherwise.

I can back out this patch tonight and generate another log if you don't believe me :), or gather other data.  The lifetime decoupling of fun/script here works against the jsd API, for which there are no tests, so I'm hoping we can come up with something relatively straightforward. :/

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> I can back out this patch tonight and generate another log if you don't believe
> me :), or gather other data.  The lifetime decoupling of fun/script here works
> against the jsd API, for which there are no tests, so I'm hoping we can come up
> with something relatively straightforward. :/

I think this strongly suggests to back out the patch from bug 424376 and make the bug wontfix for 1.9.

Comment 9

[Brendan Eich [:brendan]]

Igor: what then to do about bug 418443 and the other bug whose number I can't find now, which were regressed by nulling compile-time function object parent slots?

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor: what then to do about bug 418443 and the other bug whose number I can't
> find now, which were regressed by nulling compile-time function object parent
> slots?

The bug 418443 happens due to cloning of function objects stored in the compiled JSScript. Prior nulling the slots if one passes the compiled script to JS_EvaluateScript with the same scope that was passed to js_CompileScript, the interpreter would see that the function had the necessary parent and avoid the cloning.

Now the cloning is always done meaning that the security manager uses the principals from the scope chain, not from the script, preventing access to dom properties.

Thus the proper fix would be to teach the security manager to rely not on the cloned function status, but rather to detect that brutally shared script and use the parent chain as a principal source only in that case. 

Comment 11

[Brendan Eich [:brendan]]

So keep the fix from bug 424982 but back out the patch for bug 424376, in short? Want to make sure everything is clear to all (including me!).

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> So keep the fix from bug 424982 but back out the patch for bug 424376, in
> short? Want to make sure everything is clear to all (including me!).

No: backing out bug 424376 would address the bug 424982 since that was a regression that made the situation worse. The last fix in bug 424982 just addresses the regression without addressing the bug 418443.

Thus the bug 418443 requires separated fix.

Comment 13

[Igor Bukanov]

This is fixed as the patch for bug 424376 was backed out.