325960 - Don't nest any interpreted function calls on C stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Currently we nest heavyweight function calls and bound-non-native-method calls using the thread (C, C++, etc.) stack.  Patch in a minute.  This is a prerequisite to adding generators or any kind of structured call/cc.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

More reviews from cc: list members are welcome.  Also testing help.  This passes the testsuite in the js shell.  As I predicted the other day, the main hit is to jsinterp.c:

* js_ComputeThis was badly parameterized, taking a mostly- (but not fully-) initialized JSStackFrame before, only for a few in and out params.  I fixed that by reparameterizing, and removing the default-constructor-rval setting that was mislocated in js_ComputeThis.

* The JS_HAS_NO_SUCH_METHOD code needs to be subroutined from js_Invoke, so that the inline_call: code in js_Interpret can use it.

* The inline_call: code was not prepared to deal with heavyweights, bound methods, or functions called with fewer actual arguments than formal parameters.  Fixing the first two cases was straightforward; fixing the last was a bit of work (see the missing and overflow computations).  I decided not to subroutine more of js_Invoke, preferring a more direct approach (both direct meaning inline, and direct meaning fewer branches and more aggressive co-allocation of optionally discontiguous stack items such as the JSInlineFrame, its vars, operands).

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Picked comment nits and a unnecessary cx-> before fp->fun.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 210991 [details] [diff] [review]
proposed fix, v2

>@@ -4534,16 +4617,17 @@ interrupt:
>+            SAVE_SP_AND_PC(fp);
>             ok = js_CheckRedeclaration(cx, obj, id, attrs, &obj2, &prop);
>@@ -4659,16 +4743,17 @@ interrupt:
>+            SAVE_SP_AND_PC(fp);
>             ok = js_CheckRedeclaration(cx, parent, id, attrs, NULL, NULL);

We should try to seperate these fixes out into other bugs. This is a fine patch otherwise. r=mrbkap

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (From update of attachment 210991 [details] [diff] [review] [edit])
> >@@ -4534,16 +4617,17 @@ interrupt:
> >+            SAVE_SP_AND_PC(fp);
> >             ok = js_CheckRedeclaration(cx, obj, id, attrs, &obj2, &prop);
> >@@ -4659,16 +4743,17 @@ interrupt:
> >+            SAVE_SP_AND_PC(fp);
> >             ok = js_CheckRedeclaration(cx, parent, id, attrs, NULL, NULL);
> 
> We should try to seperate these fixes out into other bugs. This is a fine patch
> otherwise. r=mrbkap

Checked into trunk.

I filed bug 326281 on the missing register saves.

/be

Comment 5

[Igor Bukanov]

With this patch applied I no longer can read gmail via https://gmail.google.com/. The debug build always crashes during gmail load stage after login with a stacktrace like:

#4  0xb7ec3ab3 in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#5  <signal handler called>
#6  0xb7e59f8b in js_LookupPropertyWithFlags (cx=0x8dc0f90, obj=Variable "obj" is not available.
)
    at /home/igor/w/mozilla/js/src/jsobj.c:2729
#7  0xb7e59daf in js_LookupProperty (cx=0x8dc0f90, obj=0x8ea6980, 
    id=135714736, objp=0xbff14f38, propp=0xbff14f34)
    at /home/igor/w/mozilla/js/src/jsobj.c:2656
#8  0xb7e36377 in call_enumerate (cx=0x8dc0f90, obj=0x8ea6980)
    at /home/igor/w/mozilla/js/src/jsfun.c:774
#9  0xb7e35f76 in js_PutCallObject (cx=0x8dc0f90, fp=0x8e5c4fc)
    at /home/igor/w/mozilla/js/src/jsfun.c:597
#10 0xb7e3f1f5 in js_Interpret (cx=0x8dc0f90, pc=0x91fa988 "&#65533;223\200\003&#65533;", 
    result=0xbff15210) at /home/igor/w/mozilla/js/src/jsinterp.c:2168
#11 0xb7e3ddb9 in js_Execute (cx=0x8dc0f90, chain=0x8406a40, script=0x8e810c0, 
    down=0x0, flags=0, result=0xbff152e4)
    at /home/igor/w/mozilla/js/src/jsinterp.c:1493
#12 0xb7e15b07 in JS_EvaluateUCScriptForPrincipals (cx=0x8dc0f90, 
    obj=0x8406a40, principals=0x8c276d4, chars=0x8cadae8, length=568, 
    filename=0x8c27498 "https://mail.google.com/mail/?&ik=&search=inbox&view=tl&start=0&init=1&zx=4gnuqwx72wiz", lineno=47, rval=0xbff152e4)
    at /home/igor/w/mozilla/js/src/jsapi.c:4121
#13 0xb5671544 in nsJSContext::EvaluateString (this=0x8dc1780, 
    aScript=@0xbff15444, aScopeObject=0x8406a40, aPrincipal=0x8c276d0, 
    aURL=0x8c27498 "https://mail.google.com/mail/?&ik=&search=inbox&view=tl&start=0&init=1&zx=4gnuqwx72wiz", aLineNo=47, aVersion=0x0, aRetValue=0x0, 
    aIsUndefined=0xbff153fc)
    at /home/igor/w/mozilla/dom/src/base/nsJSEnvironment.cpp:1074
#14 0xb55212e5 in nsScriptLoader::EvaluateScript (this=0x874ac10, 
    aRequest=0x8e7ce98, aScript=@0xbff15444)
    at /home/igor/w/mozilla/content/base/src/nsScriptLoader.cpp:752
#15 0xb55219f8 in nsScriptLoader::ProcessRequest (this=0x874ac10, 
    aRequest=0x8e7ce98)
    at /home/igor/w/mozilla/content/base/src/nsScriptLoader.cpp:653
#16 0xb5522e53 in nsScriptLoader::DoProcessScriptElement (this=0x874ac10, 
    aElement=0x8e7ac5c, aObserver=0x8e7ac58, aFireErrorNotification=0xbff159b8)
    at /home/igor/w/mozilla/content/base/src/nsScriptLoader.cpp:586
#17 0xb5522f7e in nsScriptLoader::ProcessScriptElement (this=0x874ac10, 
    aElement=0x8e7ac5c, aObserver=0x8e7ac58)
    at /home/igor/w/mozilla/content/base/src/nsScriptLoader.cpp:336

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Duh

I should have caught this when reviewing ... we never initialized newifp->frame.pc.

Comment 7

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

Comment on attachment 211050 [details] [diff] [review]
Duh

r=shaver

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> Created an attachment (id=211050) [edit]

With this patch gmail works again :)

Comment 9

[Brendan Eich [:brendan]]

Attachment: better fix

Sorry for the crash.  We want to crash and find any missing SAVE_SP_AND_PC calls, not hide them by initializing fp->pc to null.  If anyone sees a crash caused by this bug's fixes, please reopen again and give the stack.  It will be easy to fix any remaining holes where control flows out of the interpreter into code that might reenter the interpreter.

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed again, thanks to mrbkap for covering (he r+'d the last patch).

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: another followup fix

We want the common case to be fast, so duplicating SAVE_* is ok.  We also, and this may be historically broken, want the debugger hook to see initialized frame on inline_call: entry.  It doesn't and has not (e.g., the js_GetCallObject call in js_Invoke and now in the inline_call: code happens after the hook call), but it at least needs to see a null fp->pc.

/be

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Created an attachment (id=211052) [edit]
> better fix
> 
> Sorry for the crash.  We want to crash and find any missing SAVE_SP_AND_PC
> calls, not hide them by initializing fp->pc to null.

Just to clarify: by initializing pc to null we make js_LookupPropertyWithFlags and other bytecode-inspecting call-outs from the interpreter (which null-defend) skip any special operation based on the current opcode (e.g., JSNewResolveOp with the right JSRESOLVE_* flags).  This would be a hard bug to find.  Again, better to crash on bogus pc (DEBUG build guarantees 0xda fill pattern in stack arenas).

/be