Don't nest any interpreted function calls on C stack

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 1

12 years ago
Created attachment 210937 [details] [diff] [review]
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
Attachment #210937 - Flags: superreview?(shaver)
Attachment #210937 - Flags: review?(mrbkap)
(Assignee)

Comment 2

12 years ago
Created attachment 210991 [details] [diff] [review]
proposed fix, v2

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

/be
Attachment #210937 - Attachment is obsolete: true
Attachment #210991 - Flags: superreview?(shaver)
Attachment #210991 - Flags: review?(mrbkap)
Attachment #210937 - Flags: superreview?(shaver)
Attachment #210937 - Flags: review?(mrbkap)
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
Attachment #210991 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 4

12 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 5

12 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 211050 [details] [diff] [review]
Duh

I should have caught this when reviewing ... we never initialized newifp->frame.pc.
Attachment #211050 - Flags: review?(brendan)
Comment on attachment 211050 [details] [diff] [review]
Duh

r=shaver
Attachment #211050 - Flags: review?(brendan) → review+

Comment 8

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

With this patch gmail works again :)

(Assignee)

Comment 9

12 years ago
Created attachment 211052 [details] [diff] [review]
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
Attachment #211052 - Flags: review+
(Assignee)

Comment 10

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

/be
(Assignee)

Updated

12 years ago
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

12 years ago
Created attachment 211053 [details] [diff] [review]
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
Attachment #211053 - Flags: review?(mrbkap)

Updated

12 years ago
Attachment #211053 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

12 years ago
(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

Updated

12 years ago
Flags: testcase-
Attachment #210991 - Flags: superreview?(shaver)
You need to log in before you can comment on or make changes to this bug.