Closed Bug 377090 Opened 14 years ago Closed 14 years ago

Introduce API to null out and restore cx->fp on a JSContext

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8.0.13, fixed1.8.1.5)

Attachments

(5 files, 5 obsolete files)

See bug 371858.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Assignee: general → mrbkap
Attached patch Proposed patch, v1 (obsolete) — Splinter Review
Brendan had proposed some names, but I've forgotten those. So here's one proposal.
Attachment #261209 - Flags: review?(brendan)
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 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
Attachment #261209 - Flags: review?(brendan) → review-
Attached patch Fixed patch (obsolete) — Splinter Review
Yikes, I must have diff'd that patch before saving -- it was missing jsdbgapi.c altogether. I've actually looked at this diff!
Attachment #261209 - Attachment is obsolete: true
Attachment #261353 - Flags: review?(brendan)
Attached patch With *everything* fixed (obsolete) — Splinter Review
I missed the assert with that last patch.
Attachment #261353 - Attachment is obsolete: true
Attachment #261354 - Flags: review?(brendan)
Attachment #261353 - Flags: review?(brendan)
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
Attachment #261354 - Attachment is obsolete: true
Attachment #261384 - Flags: review?(brendan)
Attachment #261354 - Flags: review?(brendan)
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
Attachment #261384 - Flags: review?(brendan) → review+
Will approve a branch patch after trunk landing, but not blocking
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Fix checked into trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to JS_RestoreFrameChain?  If so, why bother with the arg?
(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
Attached patch FollowupSplinter Review
Attachment #261626 - Flags: review?(brendan)
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 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
Attachment #261626 - Flags: review?(brendan) → review+
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.
JS_SetAsideStack/JS_RestoreStack ?
I mentioned "set-aside" but didn't proffer an antonym.

JS_SetAsideStack/JS_PutBackStack?

/be
I could live with that.
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
So given that branch freeze is Friday, what should I do with bug 371858?  Use this API?  Or wait for the other one?
mrbkap must be busy -- college boy!

Ok, revised patch next.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #261829 - Flags: review?(mrbkap)
Attached patch better followup fix (obsolete) — Splinter Review
Just the missing assertion in JS_RestoreFrameChain.

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

/be
Attachment #261832 - Flags: review?(mrbkap)
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....
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.
The followup fix is now checked into trunk.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Attachment #261829 - Attachment is obsolete: true
Attachment #261829 - Flags: review?(mrbkap)
Attachment #261832 - Attachment is obsolete: true
Attachment #261832 - Flags: review?(mrbkap)
Comment on attachment 261626 [details] [diff] [review]
Followup

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

/be
Flags: in-testsuite-
So could we get this in on branch too?
Attached patch Rolled-up patchSplinter Review
Sadly, this only applies to the 1.8.1 branch.
Attachment #264128 - Flags: approval1.8.1.5?
Attached patch 1.8.0 patchSplinter Review
Trivially hand-merged.
Attachment #264129 - Flags: approval1.8.0.13?
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
Attachment #264128 - Flags: approval1.8.1.5? → approval1.8.1.5+
Attachment #264129 - Flags: approval1.8.0.13? → approval1.8.0.13+
Fix landed on behalf of bz on both branches.
You need to log in before you can comment on or make changes to this bug.