Last Comment Bug 377090 - Introduce API to null out and restore cx->fp on a JSContext
: Introduce API to null out and restore cx->fp on a JSContext
Status: RESOLVED FIXED
: fixed1.8.0.13, fixed1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
Depends on:
Blocks: 371858
  Show dependency treegraph
 
Reported: 2007-04-10 17:54 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2007-06-22 16:14 PDT (History)
4 users (show)
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch, v1 (1.06 KB, patch)
2007-04-10 20:14 PDT, Blake Kaplan (:mrbkap)
brendan: review-
Details | Diff | Splinter Review
Fixed patch (2.44 KB, patch)
2007-04-12 00:03 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
With *everything* fixed (2.43 KB, patch)
2007-04-12 00:05 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Addressing comments (5.09 KB, patch)
2007-04-12 08:37 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
What I'm checking in (5.21 KB, patch)
2007-04-13 12:19 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Followup (709 bytes, patch)
2007-04-15 18:52 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
alternative API, but see comment below (5.61 KB, patch)
2007-04-17 12:39 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
better followup fix (711 bytes, patch)
2007-04-17 12:43 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
Rolled-up patch (5.22 KB, patch)
2007-05-08 10:26 PDT, Blake Kaplan (:mrbkap)
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review
1.8.0 patch (5.23 KB, patch)
2007-05-08 10:29 PDT, Blake Kaplan (:mrbkap)
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2007-04-10 17:54:15 PDT
See bug 371858.
Comment 1 Blake Kaplan (:mrbkap) 2007-04-10 20:14:11 PDT
Created attachment 261209 [details] [diff] [review]
Proposed patch, v1

Brendan had proposed some names, but I've forgotten those. So here's one proposal.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-11 19:01:04 PDT
Doesn't the GC need to be able to reach all the suspended cg->fp's in the case where GC runs while some frames are suspended? IOW, shouldn't the JSContext keep a reference to the suspened fp and mark that when GC'ing?
Comment 3 Brendan Eich [:brendan] 2007-04-11 20:55:25 PDT
Comment on attachment 261209 [details] [diff] [review]
Proposed patch, v1

> JS_PUBLIC_API(JSStackFrame *)
>+JS_SuspendFrameExecution(JSContext *cx)

JS_SaveFrameChain and JS_RestoreFrameChain are better names I think.

Documentation should note how they must be used LIFO with the cx's stack balanced in between.

>+{
>+    JSStackFrame *fp;
>+
>+    fp = cx->fp;
>+    fp->dormantNext = cx->dormantFrameChain;

As jst notes, you must then set cx->dormantFrameChain = fp.

>+    cx->fp = NULL;
>+    return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_ResumeFrameExecution(JSContext *cx, JSStackFrame *fp)
>+{
>+    JS_ASSERT(cx->fp == NULL);

!cx->fp is the local C idiom.

>+
>+    cx->fp = fp;
>+    cx->dormantFrameChain = fp->dormantNext;
>+    fp->dormantNext = NULL;
>+}

The rest looks right.

/be
Comment 4 Blake Kaplan (:mrbkap) 2007-04-12 00:03:17 PDT
Created attachment 261353 [details] [diff] [review]
Fixed patch

Yikes, I must have diff'd that patch before saving -- it was missing jsdbgapi.c altogether. I've actually looked at this diff!
Comment 5 Blake Kaplan (:mrbkap) 2007-04-12 00:05:15 PDT
Created attachment 261354 [details] [diff] [review]
With *everything* fixed

I missed the assert with that last patch.
Comment 6 Brendan Eich [:brendan] 2007-04-12 01:16:28 PDT
Comment on attachment 261354 [details] [diff] [review]
With *everything* fixed

>Index: jsdbgapi.c

I wonder whether we shouldn't add these to jsapi.h and move JSStackFrame's typedef from jsprvtd.h to jspubtd.h.

> JS_PUBLIC_API(JSStackFrame *)
>+JS_SaveFrameChain(JSContext *cx)
>+{
>+    JSStackFrame *fp;
>+
>+    fp = cx->fp;

Null-check fp and return it early to relieve callers from having to worry about whether cx->fp is null.

JS_ASSERT(!fp->dormantNext) here.

>+    fp->dormantNext = cx->dormantFrameChain;
>+    cx->dormantFrameChain = fp;
>+    cx->fp = NULL;
>+    return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_RestoreFrameChain(JSContext *cx, JSStackFrame *fp)
>+{
>+    JS_ASSERT(!cx->fp);

if (!fp) return.

>+
>+    cx->fp = fp;
>+    cx->dormantFrameChain = fp->dormantNext;
>+    fp->dormantNext = NULL;
[...]
> /*
>+ * Saving and restoring frame chains.
>+ *
>+ * These two functions are used to set aside cx->fp while that frame is
>+ * inactive. After a call to JS_SaveFrameChain, it looks as if there is no
>+ * code running on cx. Before calling JS_RestoreFrameChain, cx's call stack
>+ * must be balanced and all nested calls to JS_SaveFrameChain must have had
>+ * matching JS_RestoreFrameChain calls.

Note !cx->fp tolerance in separate paragraph.

/be
Comment 7 Blake Kaplan (:mrbkap) 2007-04-12 08:37:01 PDT
Created attachment 261384 [details] [diff] [review]
Addressing comments
Comment 8 Brendan Eich [:brendan] 2007-04-12 10:44:33 PDT
Comment on attachment 261384 [details] [diff] [review]
Addressing comments

>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.311
>diff -p -U8 -r3.311 jsapi.c
>--- jsapi.c	10 Apr 2007 13:29:10 -0000	3.311
>+++ jsapi.c	12 Apr 2007 15:36:03 -0000
>@@ -4416,16 +4416,43 @@ JS_PUBLIC_API(void)
> JS_SetCallReturnValue2(JSContext *cx, jsval v)
> {
> #if JS_HAS_LVALUE_RETURN
>     cx->rval2 = v;
>     cx->rval2set = JS_TRUE;
> #endif
> }
> 
>+JS_PUBLIC_API(JSStackFrame *)
>+JS_SaveFrameChain(JSContext *cx)
>+{
>+    JSStackFrame *fp;
>+
>+    fp = cx->fp;
>+    if (!fp)
>+        return fp;

Nit: extra blank line here.

>+    JS_ASSERT(!fp->dormantNext);
>+    fp->dormantNext = cx->dormantFrameChain;
>+    cx->dormantFrameChain = fp;
>+    cx->fp = NULL;
>+    return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_RestoreFrameChain(JSContext *cx, JSStackFrame *fp)
>+{
>+    JS_ASSERT(!cx->fp);
>+
>+    if (!fp)
>+        return;

Personally I would cuddle the assertion and the if-return and put the blank line after the return.

>+    cx->fp = fp;
>+    cx->dormantFrameChain = fp->dormantNext;
>+    fp->dormantNext = NULL;
>+}
>+
> /************************************************************************/
> 
> JS_PUBLIC_API(JSString *)
> JS_NewString(JSContext *cx, char *bytes, size_t nbytes)
> {
>     size_t length = nbytes;
>     jschar *chars;
>     JSString *str;
>Index: jsapi.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.h,v
>retrieving revision 3.141
>diff -p -U8 -r3.141 jsapi.h
>--- jsapi.h	7 Apr 2007 01:20:28 -0000	3.141
>+++ jsapi.h	12 Apr 2007 15:36:03 -0000
>@@ -1843,16 +1843,35 @@ JS_IsAssigning(JSContext *cx);
>  * type value (obj, id).  Only native methods can return reference types,
>  * and if the returned value is used on the left-hand side of an assignment
>  * op, the identified property will be set.  If the return value is in an
>  * r-value, the interpreter just gets obj[id]'s value.
>  */
> extern JS_PUBLIC_API(void)
> JS_SetCallReturnValue2(JSContext *cx, jsval v);
> 
>+/*
>+ * Saving and restoring frame chains.
>+ *
>+ * These two functions are used to set aside cx->fp while that frame is
>+ * inactive. After a call to JS_SaveFrameChain, it looks as if there is no
>+ * code running on cx. Before calling JS_RestoreFrameChain, cx's call stack
>+ * must be balanced and all nested calls to JS_SaveFrameChain must have had
>+ * matching JS_RestoreFrameChain calls.
>+ *
>+ * JS_SaveFrameChain deals with cx not having any code running on it. A NULL

Comments use null, not NULL, as the spelling.

r=me with these nits picked.

/be
Comment 9 Daniel Veditz [:dveditz] 2007-04-13 10:55:21 PDT
Will approve a branch patch after trunk landing, but not blocking
Comment 10 Blake Kaplan (:mrbkap) 2007-04-13 12:19:29 PDT
Created attachment 261489 [details] [diff] [review]
What I'm checking in
Comment 11 Blake Kaplan (:mrbkap) 2007-04-13 12:20:34 PDT
Fix checked into trunk.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2007-04-15 17:52:30 PDT
Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to JS_RestoreFrameChain?  If so, why bother with the arg?
Comment 13 Brendan Eich [:brendan] 2007-04-15 18:49:20 PDT
(In reply to comment #12)
> Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to
> JS_RestoreFrameChain?  If so, why bother with the arg?

I dreamed there was an assertion to that effect. The arg is a good idea so we can assert that condition, but otherwise unnecessary. mrbkap, can you do a followup patch quickly? Thanks, bz.

/be
Comment 14 Blake Kaplan (:mrbkap) 2007-04-15 18:52:06 PDT
Created attachment 261626 [details] [diff] [review]
Followup
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2007-04-15 18:53:28 PDT
I guess it's just that the arg makes my life harder in bug 371858 -- I have to change data structures to store that arg...  if it were not there, I would only need the cx, which I have.
Comment 16 Brendan Eich [:brendan] 2007-04-15 18:56:32 PDT
Comment on attachment 261626 [details] [diff] [review]
Followup

Thanks. In light of bz's observation, perhaps we should have kept JSStackFrame private, as the new API begs the question: how does the GC know to scan the frame chain being set aside?

/be
Comment 17 Blake Kaplan (:mrbkap) 2007-04-15 19:02:18 PDT
It's not too late to undo that stuff. I think that the API that bz wants is something like:

JS_PUBLIC_API(void)
JS_SaveStackFrame(JSContext *cx);

JS_PUBLIC_API(void)
JS_RestoreStackFrame(JSContext *cx);

Which seems very magical to me (what's being saved, what's being restored?), but it isn't like the implementation of JS_RestoreStackFrame is going to change to actually need fp, ever.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2007-04-15 19:07:29 PDT
JS_SetAsideStack/JS_RestoreStack ?
Comment 19 Brendan Eich [:brendan] 2007-04-15 19:51:04 PDT
I mentioned "set-aside" but didn't proffer an antonym.

JS_SetAsideStack/JS_PutBackStack?

/be
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2007-04-15 19:54:21 PDT
I could live with that.
Comment 21 Brendan Eich [:brendan] 2007-04-15 22:56:29 PDT
Hard to riff on the "dormant" metaphor tonight for some reason -- JS_Sleep and JS_Wakeup are wrong for so many reasons (they suggest thread or process scheduling from Unix). So I say it's comment 19 or bust -- mrbkap, want to reopen and patch one more time?

/be
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2007-04-17 12:15:09 PDT
So given that branch freeze is Friday, what should I do with bug 371858?  Use this API?  Or wait for the other one?
Comment 23 Brendan Eich [:brendan] 2007-04-17 12:24:38 PDT
mrbkap must be busy -- college boy!

Ok, revised patch next.

/be
Comment 24 Brendan Eich [:brendan] 2007-04-17 12:39:21 PDT
Created attachment 261829 [details] [diff] [review]
alternative API, but see comment below

I am now thinking the API checked in was the right one (sorry, bz). There's no safe way to handle the !cx->fp case without at least one bit of return value from JS_SetAsideStack, telling whether or not to call JS_PutBackStack. Otherwise one could have !cx->fp && cx->dormantFrameChain upon calling JS_SetAsideStack, which would return (void, in the proposal) right away. But JS_PutBackStack would then wrongly put back cx->dormantFrameChain as cx->fp.

With this patch, bz, you'd need to store a boolean or otherwise remember not to put back after a false-returning set aside call. That's hardly better than having to save fp, and saving fp allows us to sanity check cx->dormantFrameChain in put-back. So I'll revert these changes and propose another patch. mrbkap, feel free to minus this one.

/be
Comment 25 Brendan Eich [:brendan] 2007-04-17 12:43:47 PDT
Created attachment 261832 [details] [diff] [review]
better followup fix

Just the missing assertion in JS_RestoreFrameChain.

I kept the save and restore trope, and the concrete "frame chain" idiom.

/be
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2007-04-17 12:55:14 PDT
Yeah, storing a boolean is just as much work as storing a JSStackFrame.

So I'll run with this API.  I hadn't thought about the problem of no fp but having a dormantFrameChain....
Comment 27 Blake Kaplan (:mrbkap) 2007-04-17 14:06:42 PDT
I'm just going to check in the followup fix that you already stamped. Sorry for the delay. It's the second to last week of classes and all of my projects and papers are coming up.
Comment 28 Blake Kaplan (:mrbkap) 2007-04-17 14:07:45 PDT
The followup fix is now checked into trunk.
Comment 29 Brendan Eich [:brendan] 2007-04-17 14:15:14 PDT
Comment on attachment 261626 [details] [diff] [review]
Followup

Wahhh, I liked my fp on the left of == assertion :-P.

/be
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2007-05-04 22:56:09 PDT
So could we get this in on branch too?
Comment 31 Blake Kaplan (:mrbkap) 2007-05-08 10:26:32 PDT
Created attachment 264128 [details] [diff] [review]
Rolled-up patch

Sadly, this only applies to the 1.8.1 branch.
Comment 32 Blake Kaplan (:mrbkap) 2007-05-08 10:29:41 PDT
Created attachment 264129 [details] [diff] [review]
1.8.0 patch

Trivially hand-merged.
Comment 33 Daniel Veditz [:dveditz] 2007-06-22 11:37:06 PDT
Comment on attachment 264128 [details] [diff] [review]
Rolled-up patch

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2007-06-22 16:14:50 PDT
Fix landed on behalf of bz on both branches.

Note You need to log in before you can comment on or make changes to this bug.