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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bz, Assigned: mrbkap)

Tracking

({fixed1.8.0.13, fixed1.8.1.5})

Trunk
x86
Mac OS X
fixed1.8.0.13, fixed1.8.1.5
Points:
---
Bug Flags:
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
See bug 371858.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?

Updated

10 years ago
Assignee: general → mrbkap
(Assignee)

Comment 1

10 years ago
Created attachment 261209 [details] [diff] [review]
Proposed patch, v1

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-
(Assignee)

Comment 4

10 years ago
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!
Attachment #261209 - Attachment is obsolete: true
Attachment #261353 - Flags: review?(brendan)
(Assignee)

Comment 5

10 years ago
Created attachment 261354 [details] [diff] [review]
With *everything* fixed

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
(Assignee)

Comment 7

10 years ago
Created attachment 261384 [details] [diff] [review]
Addressing comments
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?
(Assignee)

Comment 10

10 years ago
Created attachment 261489 [details] [diff] [review]
What I'm checking in
(Assignee)

Comment 11

10 years ago
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

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

Comment 14

10 years ago
Created attachment 261626 [details] [diff] [review]
Followup
Attachment #261626 - Flags: review?(brendan)
(Reporter)

Comment 15

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

Comment 17

10 years ago
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.
(Reporter)

Comment 18

10 years ago
JS_SetAsideStack/JS_RestoreStack ?
I mentioned "set-aside" but didn't proffer an antonym.

JS_SetAsideStack/JS_PutBackStack?

/be
(Reporter)

Comment 20

10 years ago
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
(Reporter)

Comment 22

10 years ago
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 → ---
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
Attachment #261829 - Flags: review?(mrbkap)
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

Updated

10 years ago
Attachment #261832 - Flags: review?(mrbkap)
(Reporter)

Comment 26

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

Comment 27

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

Comment 28

10 years ago
The followup fix is now checked into trunk.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #261829 - Attachment is obsolete: true
Attachment #261829 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
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-
(Reporter)

Comment 30

10 years ago
So could we get this in on branch too?
(Assignee)

Comment 31

10 years ago
Created attachment 264128 [details] [diff] [review]
Rolled-up patch

Sadly, this only applies to the 1.8.1 branch.
Attachment #264128 - Flags: approval1.8.1.5?
(Assignee)

Comment 32

10 years ago
Created attachment 264129 [details] [diff] [review]
1.8.0 patch

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.
Keywords: fixed1.8.0.13, fixed1.8.1.5
You need to log in before you can comment on or make changes to this bug.