354392 - Refreshing a page while using ActiveState's JavaScript debugger often results in a Firefox crash [@ js_DecompileScript]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Promislow]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060731 Firefox/1.5.0.5 Flock/0.7.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

I'm working with two instances of 1.5.0.7 -- the standard retail
version I downloaded, and a separate instance I build from source
in debug mode so I could debug inside Visual Studio.

With either one, once I establish a connection between the ActiveState
debugger and Komodo, often refreshing certain kinds of pages will cause Firefox
to crash.  The stack trace suggests that the data structure contains a
stale field which is probably often correct, but sometimes isn't filled in
correctly until the debugger hook has returned.  The crash occurs while
the scriptLoaded hook is executing.  Stack trace follows in the "Additional Information" field.

Reproducible: Always

Steps to Reproduce:
1. Start up Komodo, have listen on port xxxx for a debugger connection
2. Start up Firefox (Windows only) with the ActiveState JS Debugger extension installed.
3. Switch to a page with significant JS processing, like http://www.feertech.com/jsfractals.html.  Start debugging.  Press "Continue" at some point in Komodo, then return to Firefox and press refresh.  This started triggering a crash sometime between 1.5.0 and the 1.5.0.7 upgrade.

Actual Results:  
Firefox crashes.

Expected Results:  
The page should refresh.

My scriptCreated hook fires at this line and dies
(using the jsdIDebuggerService API):

    if (!jsdScript.isValid) {
        return;
    }
// So we know that the script object is valid
    try {
        var functionObject = jsdScript.functionObject;
        if (!functionObject || functionObject.jsType != functionObject.TYPE_FUNCTION) {
            script_ok = false;
        } else {
// And we're definitely looking at a function object
            this.log.debug("**************** -- functionObject.jsType = "
                           + functionObject.jsType
                           + ", going for functionSource...")
*CRASH* ==> script_ok = jsdScript.functionSource && jsdScript.functionSource.length > 0;
            this.log.debug("**************** got it");
        }

I'm going to dump a big stack trace here.  The important parts are the top two calls
(it crashes walking a null pointer, and we see the null pointer passed to the
callee at jsopcode.c, line 2897.  Then move down to my last comment on the stack
trace, where it looks to me like the fun object is corrupt, where not all fields
are set before it gets used.  I'll mark this discussion with "****". 

Firefox crashes in jsopcode.c, line 2792
JSBool
js_DecompileScript(JSPrinter *jp, JSScript *script)
{
====>     return js_DecompileCode(jp, script, script->code, (uintN)script->length);
}

Reason for crash: script is set to the NULL pointer.

Traversing the stack:
(Given above)
     js3250.dll!js_DecompileScript(JSPrinter * jp=0x04b3b580, JSScript * script=0x00000000)  Line 2792 + 0x3    C

jsopcode.c : 2897
====>  ok = js_DecompileScript(jp, fun->u.script);
    js3250.dll!js_DecompileFunction(JSPrinter * jp=0x04b3b580, JSFunction * fun=0x04d53040)  Line 2897 + 0x10    C


jsapi.c : 3978
====>     if (js_DecompileFunction(jp, fun))
        str = js_GetPrinterOutput(jp);
    else
        str = NULL;

     js3250.dll!JS_DecompileFunction(JSContext * cx=0x030043d0, JSFunction * fun=0x04d53040, unsigned int indent=4)  Line 3978 + 0xd    C

jsd_xpc.cpp : 1268
    if (fun)
    {
====>        jsstr = JS_DecompileFunction (cx, fun, 4);
    }
    else
    {
        JSScript *script = JSD_GetJSScript (mCx, mScript);
        jsstr = JS_DecompileScript (cx, script, "ppscript", 4);
    }

     jsd3250.dll!jsdScript::GetFunctionSource(nsAString_internal & aFunctionSource={...})  Line 1268 + 0x10    C++

/// Now we're in the JS engine...
    xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x04c62208, unsigned int methodIndex=14, unsigned int paramCount=1, nsXPTCVariant * params=0x0012ddd0)  Line 102    C++

     xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_GETTER)  Line 2152 + 0x1e    C++

     xpc3250.dll!XPCWrappedNative::GetAttribute(XPCCallContext & ccx={...})  Line 1953 + 0xe    C++

     xpc3250.dll!XPC_WN_GetterSetter(JSContext * cx=0x041093c0, JSObject * obj=0x04b035d8, unsigned int argc=0, long * argv=0x04f214dc, long * vp=0x0012e0c0)  Line 1477 + 0xc    C++
     js3250.dll!js_Invoke(JSContext * cx=0x041093c0, unsigned int argc=0, unsigned int flags=2)  Line 1188 + 0x20    C
     js3250.dll!js_InternalInvoke(JSContext * cx=0x041093c0, JSObject * obj=0x04b035d8, long fval=78657328, unsigned int flags=0, unsigned int argc=0, long * argv=0x00000000, long * rval=0x0012ec38)  Line 1285 + 0x14    C
     js3250.dll!js_InternalGetOrSet(JSContext * cx=0x041093c0, JSObject * obj=0x04b035d8, long id=50500200, long fval=78657328, JSAccessMode mode=JSACC_READ, unsigned int argc=0, long * argv=0x00000000, long * rval=0x0012ec38)  Line 1344 + 0x1f    C
     js3250.dll!js_GetProperty(JSContext * cx=0x041093c0, JSObject * obj=0x04b035d8, long id=50500200, long * vp=0x0012ec38)  Line 3001 + 0x30    C
     js3250.dll!js_Interpret(JSContext * cx=0x041093c0, unsigned char * pc=0x030e5495, long * result=0x0012ed88)  Line 3408 + 0x62e    C
     js3250.dll!js_Invoke(JSContext * cx=0x041093c0, unsigned int argc=1, unsigned int flags=2)  Line 1208 + 0x13    C
     xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x047c8598, unsigned short methodIndex=3, const nsXPTMethodInfo * info=0x02f49a90, nsXPTCMiniVariant * nativeParams=0x0012f0d4)  Line 1373 + 0x14    C++
     xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=3, const nsXPTMethodInfo * info=0x02f49a90, nsXPTCMiniVariant * params=0x0012f0d4)  Line 462    C++
     xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x047c8598, unsigned int methodIndex=3, unsigned int * args=0x0012f19c, unsigned int * stackBytesToPop=0x0012f18c)  Line 117 + 0x1c    C++
     xpcom_core.dll!SharedStub()  Line 147    C++

jsd_xpc.cpp : 726
====> hook->OnScriptCreated (script);
     jsd3250.dll!jsds_ScriptHookProc(JSDContext * jsdc=0x02ea4408, JSDScript * jsdscript=0x04bd3700, int creating=1, void * callerdata=0x00000000)  Line 727    C++

jsd_scpt.c : 589 -- nothing too interesting here
if( hook )
====>      hook(jsdc, jsdscript, JS_TRUE, hookData);
    jsd3250.dll!jsd_NewScriptHookProc(JSContext * cx=0x041093c0, const char * filename=0x0472a039, unsigned int lineno=1, JSScript * script=0x04f16040, JSFunction * fun=0x04d53040, void * callerdata=0x02ea4408)  Line 589 + 0x11    C


jsscript.c : 1308
JS_FRIEND_API(void)
js_CallNewScriptHook(JSContext *cx, JSScript *script, JSFunction *fun)
{
    JSRuntime *rt;
    JSNewScriptHook hook;

    rt = cx->runtime;
    hook = rt->newScriptHook;
    if (hook) {
        JS_KEEP_ATOMS(rt);
        hook(cx, script->filename, script->lineno, script, fun,
             rt->newScriptHookData);
        JS_UNKEEP_ATOMS(rt);
    }
}
      js3250.dll!js_CallNewScriptHook(JSContext * cx=0x041093c0, JSScript * script=0x04f16040, JSFunction * fun=0x04d53040)  Line 1308 + 0x27    C

jsscript.c : 1289
    /* Tell the debugger about this compiled script. */
====>    js_CallNewScriptHook(cx, script, fun);
    return script;

     js3250.dll!js_NewScriptFromCG(JSContext * cx=0x041093c0, JSCodeGenerator * cg=0x04f420b0, JSFunction * fun=0x04d53040)  Line 1289 + 0x11    C

jsemit.c : 2788
====>    fun->u.script = js_NewScriptFromCG(cx, cg, fun);
    if (!fun->u.script)
        return JS_FALSE;

     js3250.dll!js_EmitFunctionBody(JSContext * cx=0x041093c0, JSCodeGenerator * cg=0x04f420b0, JSParseNode * body=0x04e8a3f0, JSFunction * fun=0x04d53040)  Line 2788 + 0x11    C

**** This is the thing I don't understand.  The variable called "fun" in js_EmitFunctionBody
is the same one as the variable with the same name at level 2 of the stack trace in
js_DecompileFunction. It looks to me that fun->u.script is assigned to the
underlying script only after the debugger has been notified.  So the debugger hook is
working with an incompletely initialized object.

If we aren't debugging, we never call rt->newScriptHook, so it doesn't
matter when fun->u.script is updated.

Since I execute this many times while running code, I'm guessing that this
field is set correctly when a page is loaded, but a recent change broke 
setting this field on refresh.

Comment 1

[Brendan Eich [:brendan]]

It looks like the new script hook has *always* been called from the code generator with script and fun arguments such that fun->u.script is null (initialized null by default) until the js_NewScriptFromCG call unwinds to js_EmitFunctionBody and the return value JSScript * is stored in fun->u.script.

This is bogus, considering how the new script hook is called with fun->u.script set to its script from fun_xdrObject.

I have no idea why things seemed to work in older versions.  It's hard to believe given the long-standing control flow.  Perhaps jsd had another way of coping and lost it?

The fix is trivial: make js_NewScriptFromCG test for non-null fun and set fun->u.script before calling the new script hook.  To save a redundant store, change js_EmitFunctionBody to simply test the return value of js_NewScriptFromCG and return false if it's null.

Crowder, can you patch quickly?  This is a good one to get into 1.8.0.8 and well as 1.8.1 and trunk.

/be

Comment 2

[Brian Crowder]

Attachment: The patch described above

This patch doesn't concern itself with whether the function passed in was already initialized with a script.  Should it?

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 240264 [details] [diff] [review]
The patch described above

>+    /*
>+     * We initialize fun->u.script to be the script constructed above
>+     * so that the debugger has a valid script to work with.

"has a valid FUN_SCRIPT(fun)."

>+     */
>+    if (fun)
>+        fun->u.script = script;

Nit: blank line here.

>     /* Tell the debugger about this compiled script. */
>     js_CallNewScriptHook(cx, script, fun);
>     return script;
> 
> bad:
>     js_DestroyScript(cx, script);
>     return NULL;
> }

(In reply to comment #2)
> Created an attachment (id=240264) [edit]
> The patch described above
> 
> This patch doesn't concern itself with whether the function passed in was
> already initialized with a script.  Should it?

No, the contract is that the function is newly created.  It could be asserted that FUN_INTERPRETED(fun) && !FUN_SCRIPT(fun) on the trunk (change FUN_INTERPRETED(fun) to fun->interpreted to port to the 1.8.0 branch).

How about the jsemit.c change to avoid redundantly storing fun->u.i.script (fun->u.script).

/be

Comment 4

[Brian Crowder]

Attachment: Initialize script pointer for functions in js_NewScriptFromGC (trunk only)

Added a similar redundant-store removal for jsparse.c

1.8 compatible patch (just rewording the assertion) coming next, if this one gets r+

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 240274 [details] [diff] [review]
Initialize script pointer for functions in js_NewScriptFromGC (trunk only)

Great, thanks!

1.8.0 branch patch should be easy.

/be

Comment 6

[Brian Crowder]

Attachment: same as above, tailored to 1_8_0 branch

Comment 7

[Mike Schroepfer]

Is this codepath only exercised under debugging - or is it touched in other circumstances?  What's the risk of the patch at this stage of the game?

Comment 8

[Eric Promislow]

This only happens during debugging(In reply to comment #7)
> Is this codepath only exercised under debugging - or is it touched in other
> circumstances?  What's the risk of the patch at this stage of the game?
> 

If there's no new-script hook, the js_DecompileScript can't possibly
be invoked.  And if there were a hook, the hook code would
have to access jsdIScript.functionSource to hit the bug.

I'm building a version of 1.0.5.7 with the patches submitted to see
if I can repro.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Is this codepath only exercised under debugging - or is it touched in other
> circumstances?  What's the risk of the patch at this stage of the game?

This is a safe patch.  The change to store the script in the function simply moves the store from one place along straight-line (in the absence of errors that kill the entire compilation) control flow to a slightly earlier place, where the debugger hook can then find the script from the function.

/be

Comment 10

[Eric Promislow]

When I apply patch https://bugzilla.mozilla.org/attachment.cgi?id=240277&action=view
I get a JS assertion failure while starting up firefox at jsscript.c:1298:
    if (fun) {
====>        JS_ASSERT(fun->interpreted && !FUN_SCRIPT(fun));
        fun->u.script = script;
    }

Current state:
filename="javascript:return new XPCNativeWrapper(arg);"
fun.flags = 0

Comment 11

[Eric Promislow]

I removed the assertion at line 1298 of jsscript.c,
rebuilt, and the debugged Firefox no longer crashes on refresh.

What's the purpose of that assertion?  If this patch is
nothing more than a reordering of independent actions on
linear code paths, what purpose is it serving?

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> I removed the assertion at line 1298 of jsscript.c,
> rebuilt, and the debugged Firefox no longer crashes on refresh.
> 
> What's the purpose of that assertion?  If this patch is
> nothing more than a reordering of independent actions on
> linear code paths, what purpose is it serving?

In the future, please say which term in the assertion was false.

The assertion is making a point about the function not being initialized with another script already, but being an interpreted function, so being ready to have the script stored in it.

It looks like fun->interpreted is not set early enough in js_CompileFunctionBody on the 1.8.0 branch.  That is why the assertion is botching (so the term that's false is the first one).

This was fixed on the trunk and the 1.8 branch as part of the fix for bug 346892.  That bug's fix could be backported easily.  It's not a critical patch for the 1.8.0 branch, however, so losing the assertion on the 1.8.0 branch may be best.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Created an attachment (id=240277) [edit]
> same as above, tailored to 1_8_0 branch

The hazards of blind backporting :-/.

Anyway, the assertion botching on the 1.8.0 branch build that Eric made is not a big deal, and has no bearing on the trunk and 1.8 branch versions of the patch.

Again, the point of the assertion is to address the question raised by crowder in comment 2.  Assertions of this kind are both good documentation and useful sanity checks (e.g., when backporting without testing! ;-).

/be

Comment 14

[Eric Promislow]

(In reply to comment #12)

Sorry, I figured that the value for 
filename of "javascript:return new XPCNativeWrapper(arg);"
would imply that fun->interpreted was 0.  I should
have been more explicit.  I didn't dig down to
understand why that should be so.

I also ruled out guarding the script assignment with a 
test on fun->interpreted (or the macro), as the old
code base didn't do that.

Let me also test this out on the 2.0 code base.  So far I've
only worked on 1.5.0.7

Comment 15

[Mike Schroepfer]

Comment on attachment 240274 [details] [diff] [review]
Initialize script pointer for functions in js_NewScriptFromGC (trunk only)

Thanks for the additional info. Approved for RC2.

Comment 16

[Brian Crowder]

Attachment: Without the assertion this time (for 1_8_0_BRANCH only)

Comment 17

[Brian Crowder]

Comment on attachment 240369 [details] [diff] [review]
Without the assertion this time (for 1_8_0_BRANCH only)

Wrong file, retrying

Comment 18

[Brian Crowder]

Attachment: No assertion (patch for 1.8.0 branch only)

Comment 19

[Brian Crowder]

(In reply to comment #14)
> Let me also test this out on the 2.0 code base.  So far I've
> only worked on 1.5.0.7
> 

Any results from this?  I can't conveniently test the fixes here.

Comment 20

[Eric Promislow]

I work on Windows, and ran into this problem
http://forums.mozillazine.org/viewtopic.php?t=455771&sid=081d4e981e2b2e7bb5685facf1f1b641
on the Firefox 2.0rc1 build.

I tried to pull perl out of cygwin, and cygwin setup
decided my install was old enough that it required a
full refresh.  About to reboot, point my path to
cygwin-perl, as per suggestions in that thread,
and try again.

Comment 21

[Eric Promislow]

The patches in https://bugzilla.mozilla.org/attachment.cgi?id=240374&action=view
solve the crash bug with the 2.0rc1 code base, as well as with 1.5.0.7
Thanks for the prompt reply on this.

I'll test this on the CVS tip tonight.  The code in this area
has changed since 2.0rc1, but I'm expecting the same results.

Comment 22

[Eric Promislow]

After many hours, my browser build off CVS ends thus:

make[6]: Leaving directory `/cygdrive/d/mozilla/mozilla/security/nss/cmd/shlibsign/mangle'
cd d:/mozilla/mozilla/ffd/nss ; sh d:/mozilla/mozilla/security/nss/cmd/shlibsign/./sign.sh d:/mozilla/mozilla/ffd/dist \
d:/mozilla/mozilla/ffd/nss WIN95 d:/mozilla/mozilla/ffd/dist/lib \
d:/mozilla/mozilla/ffd/dist/lib/softokn3.dll
d:/mozilla/mozilla/security/nss/cmd/shlibsign/./sign.sh: \
     line 2: syntax error ne'r unexpected token `in
d:/mozilla/mozilla/security/nss/cmd/shlibsign/./sign.sh: \
     line 2: `case "${3}" in '
make[5]: *** [d:/mozilla/mozilla/ffd/dist/lib/softokn3.chk] Error 2
make[5]: Leaving directory `/cygdrive/d/mozilla/mozilla/security/nss/cmd/shlibsi
gn'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/d/mozilla/mozilla/ffd/security/manager'
make[3]: *** [libs_tier_toolkit] Error 2
make[3]: Leaving directory `/cygdrive/d/mozilla/mozilla/ffd'
make[2]: *** [tier_toolkit] Error 2
make[2]: Leaving directory `/cygdrive/d/mozilla/mozilla/ffd'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/d/mozilla/mozilla/ffd'
make: *** [build] Error 2

Looks like backslash problems, and I have some other deadlines.  I'm
happy with the results I saw on 1.5.0.7 and 2.0rc1.  The rc1 code is
slightly different from the CVS code, but not in the areas the patch
deals with.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch (1.8 branch)

The merge conflict is pretty trivial to resolve, but I'd rather be safe than sorry.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Checked in on the branch.
mozilla/js/src/jsscript.c 	3.79.2.14
mozilla/js/src/jsparse.c 	3.142.2.61
mozilla/js/src/jsemit.c 	3.128.2.52

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/js/src/jsscript.c 	3.113
mozilla/js/src/jsparse.c 	3.248
mozilla/js/src/jsemit.c 	3.215

Comment 26

[Brian Crowder]

This patch is alleged to cause bug 355192, can we back it out and see if the tbox for this build goes green?

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> This patch is alleged to cause bug 355192, can we back it out and see if the
> tbox for this build goes green?

I backed out the patch from this bug for one xserve03 cycle and it didn't help, so I checked it back in.

Comment 28

[Eric Promislow]

Meanwhile I wrapped the debugger handler in a setTimeout
to work with older versions of Firefox, and that
worked around this bug.