227432 - Engine crash when calling Script.exec() from JS_CallFunctionName

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Celso Aguiar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

If calling Script.exec() from JS_CallFunctionName, a crash happens in
js_CheckRedeclaration for lack of 'obj'. The problem seems to be in script_exec,
where a varobj is not provided correctly. More details w/ a stack trace are on
an email I sent to the jseng list under the subject "Crash in Script.exec()" on
12/03/2003. Sorry I can't add them here, as the email was sent via google groups
and has not shown up yet. Here's Brendan's response to it.

From : 	Brendan Eich <[e-mail]>
Sent : 	Wednesday, December 3, 2003 8:34 PM
To : 	Celso Aguiar <[e-mail]>
Subject : 	Re: Crash in Script.exec()
Celso Aguiar wrote:
 	 	Could someone please help me with this problem?

Please file a bug against the Browser/JS Engine product/component at
http://bugzilla.mozilla.org. The Script object code is not up to snuff -- it
assumes a caller to script_exec has a varobj, and as you note, calling
script_exec via JS_CallFunctionName does not do any such set-up. You can assign
the bug to me, cc: [e-mail], and let most other fields default. Please
post the bug link as a followup. Thanks,

/be


Reproducible: Always

Steps to Reproduce:
You're going to have to call Script.exec() from JS_CallFunctionName and provide
the scope param. In my original code Script was the result of a frozen/thawed
script.


Expected Results:  
No crash

Comment 1

[Celso Aguiar]

Could someone please help me with this problem? I'm getting a crash in
js_CheckRedeclaration when calling Script.exec() from
JS_CallfunctionName. I get the crash either if I pass in a scope
object parameter to exec (my global object) or not. The script was
obtained from frozen/thaw'ed bytecode. 'obj' in js_CheckRedeclaration
is NULL, apparently because it could not find my scopeobj, despite
passing in one.
Looking at the code in script_exec:

    fp = cx->fp;
    caller = fp->down;
    if (!scopeobj)
      scopeobj = caller->scopeChain;
    fp->thisp = caller->thisp;
    JS_ASSERT(fp->scopeChain == caller->scopeChain);
    fp->sharpArray = caller->sharpArray;
    return js_Execute(cx, scopeobj, script, fp, 0, rval);

it doesn't look like js_Execute will be able to do any good if fp
and scopeobj are non-NULL. When called from JS_CallfunctionName, 
a fp->varobj will not be available, later causing the crash. 
If I null fp (down) after entering js_Execute, and leave scopeobj
(chain) alone, all goes well.
The stack is below.
Thanks, I appreciate any help.

Celso

js_CheckRedeclaration(JSContext * cx=0x031c3d70, JSObject *
obj=0x00000000, long id=0x04445670, unsigned int attrs=0x00000005, int
* foundp=0x0012bcac)  Line 1155 + 0x19 C
js_Interpret(JSContext * cx=0x031c3d70, long * result=0x0012c2a0) 
Line 3302 + 0x4e C
js_Execute(JSContext * cx=0x031c3d70, JSObject * chain=0x0320bef8,
JSScript * script=0x047b4840, JSStackFrame * down=0x0012c280, unsigned
int special=0x00000000, long * result=0x0012c2a0)  Line 1016 + 0xd C
script_exec(JSContext * cx=0x031c3d70, JSObject * obj=0x044465e0,
unsigned int argc=0x00000001, long * argv=0x047c6a48, long *
rval=0x0012c2a0)  Line 250 + 0x1b C
js_Invoke(JSContext * cx=0x031c3d70, unsigned int argc=0x00000001,
unsigned int flags=0x00000002)  Line 836 + 0x1c C
js_InternalInvoke(JSContext * cx=0x031c3d70, JSObject *
obj=0x044465e0, long fval=0x031ca3b0, unsigned int flags=0x00000000,
unsigned int argc=0x00000001, long * argv=0x047cca68, long *
rval=0x0012c3d0)  Line 928 + 0x14 C
JS_CallFunctionName(JSContext * cx=0x031c3d70, JSObject *
obj=0x044465e0, const char * name=0x0012c4e0, unsigned int
argc=0x00000001, long * argv=0x047cca68, long * rval=0x0012c3d0)  Line
3401 + 0x1f C
ESObjectCallEx(_s_ESObjectRec * eso=0x045d4a80, const char *
szName=0x0012c4e0, _s_ESObjectRec * esoArgs=0x045d4b60, _s_ESValRec *
esvRet=0x031f4b58, unsigned short bIgnorePermCheck=0x0001, unsigned
short bAllowUserCancel=0x0000)  Line 3539 + 0x24 C++

Comment 2

[Brendan Eich [:brendan]]

The fix for this case, native code calling Script.prototype.exec directly, will
have to depend on either cx->globalObject, or the parent-linked oldest ancestor
of the function object for exec, giving the correct scope object to use.  There
isn't any other object at hand, and we need something.

Celso, in which scope do you want the script to execute?  The one given by
cx->globalObject, or the one given by Script.prototype.exec.__parent__, which
may or may not be the same as cx->globalObject?

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

See the comments -- I went with cx->globalObject for the reason given.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 136842 [details] [diff] [review]
proposed fix

r=shaver with comment refinement from IRC, and a passing of the suite.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 136842 [details] [diff] [review]
proposed fix

shaver: no Script exec tests in the suite.

Looking for fast 1.6b approval.

/be

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 136842 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 7

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 8

[Phil Schwartau]

Celso: can you try this fix and see if it fixes the problem? If so,
please mark this bug "Verified". If not, you can reopen it; thanks -

Comment 9

[Celso Aguiar]

I've incorporated the changes to our engine version and didn't get any more
crashes when calling Script.exec() from JS_CallFunctionName. I'd like to notice
though that our app is not using the bleeding edge engine (in fact we're using
1.5 RC4a), so this might have been an incomplete test. Brendan changes, for
example, use JS_GetScriptedCaller, which I had to define locally to jsscript.c
in order to run/compile w/ the changes. Not sure if I should mark the bug as
"Verified".

Comment 10

[Brendan Eich [:brendan]]

script_exec hasn't changed much otherwise, and the change was local, with only
obvious dependencies such as JS_GetScriptedCaller, so I'm happy to take Celso's
word as verification.  The essential point is the crash fix via a better scope
object being given to js_Execute.

/be

Comment 11

[Celso Aguiar]

Jeez, I take that back. :( When I tested the changes I forgot to remove a
portion of my code which did a provisory fix (provided a scope, managing to call
my call to Script.exec from a JS_ExecuteScript) avoiding the crash. When I took
that out, the crash still happens and in the same place. I noticed the code path
in script_exec for when scopeobj exists (I'm passing a param to exec()) didn't
change much, so no wonder it'd still crash. In that case fp still does not
provide a varobj. Sorry for the confusion.

Comment 12

[Celso Aguiar]

My point regarding the message before is, when you get to js_Execute, if down,
frame.varobj gets updated w/ down.varobj, which is still null, and does not use
chain for varobj.

Comment 13

[Brendan Eich [:brendan]]

Celso, if you pass a non-null scopeobj from script_exec into js_Execute via the
latter's |chain| parameter, why doesn't frame.varobj get set to |obj|, which is
computed from |chain|?  Can you debug into js_Execute a bit and see what's going
wrong there?

/be

Comment 14

[Brendan Eich [:brendan]]

Oops, crossed comments (I was replying based on bugmail).  So, I thought the
caller local in script_exec (|down| in js_Execute) would be null, because there
is no scripted caller when you use JS_CallFunctionName on a script object to
call its "exec" method.  Why is caller non-null in script_exec in this case?

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed followup fix

Argh, the logic had a hole in it, and Celso must still be calling exec from a
script (but one without a varobj -- so it must be the script for a lightweight
function -- right?), and passing a scopeobj arg to exec.  In that case, caller
is non-null but caller->varobj is null, so js_Execute blithely copies that into
the new frame.varobj.

Celso, can you try this patch, and confirm that you're still calling exec via a
script (and a lightweight [no eval calls] function script at that)?

/be

Comment 16

[Celso Aguiar]

I've looked at the bleeding edge code for js_Execute and my version's and didn't
find any pertinent difference so this should be still valid. Anyway, this is my
impression of what may be happening:
If I provide chain (scopeobj) and non-NULL down (fp = cx->fp), js_Execute still
picks frame.varobj from down->varobj (which is NULL) and never goes into the
else branch. In js_Execute (edited):

    if (down) {
        frame.varobj = down->varobj;
    } else {
        obj = chain;
        frame.varobj = obj;
    }

caller IS actually null but not fp (whose fp->varobj IS null).

Comment 17

[Celso Aguiar]

I'm calling it just from JS_CallFunctionName not from a script execution,
providing a param to Script.exec(). caller is null, scopeobj and fp are non-null

Comment 18

[Brendan Eich [:brendan]]

Celso: your hand merge of jsscript.c missed a crucial change: you must pass
caller, not fp, to js_Execute from script_exec.

Do that, and your case sould work.  But there's still a logic hole: there might
be a lightweight-function-scripted call to exec that passes a scopeobj param,
leaving a null varobj being set in the exec frame.  And, as shaver pointed out,
perhaps the cx being used by the native code that calls JS_CallFunctionName has
other JS active (topped by a lightweight function call) on it, further up the C
stack?  Second patch should cope, in any event.

/be

Comment 19

[Celso Aguiar]

HaHa', the joys of not being on the edge. There was no '+' before that line in
Brendan's first change! Why? because the bleeding edge code had already changed
to using caller instead of fp.
I'll try that + the second patch. Will let you know. Thanks a bunch!

Comment 20

[Brendan Eich [:brendan]]

The edge isn't bleeding, really -- it'd be cool if you could try a trunk build,
at least for testing.  I promise it won't hurt (not compared to hand-merging
when you can't see the prior changes from the patch I put here -- sorry, I
should have thought of that!).

/be

Comment 21

[Celso Aguiar]

I've tried the latest patch and caller instead of fp. Everything works fine now. 
Thanks again!

Comment 22

[Brendan Eich [:brendan]]

Fixed.

/be