512624 - TM: Remove JSStackFrame::dormantNext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

The api for dormant frame chains is unfortunately poorly designed w.r.t. error handling. Looks like it has no way to report an oom error in JS_SaveFrameChain.

Comment 2

[Brendan Eich [:brendan]]

What's the problem?

There is no allocation under JS_SaveFrameChain, but if there were, then null return would mean that JS_ReportOutOfMemory(cx) had already been called by the JS_SaveFrameChain implementation.

/be

Comment 3

[Andreas Gal :gal]

I would like to allocate under JS_SaveFrameChain. The API says:

/*
 * Saving and restoring frame chains.
 *
 * These two functions are used to set aside cx's call stack while that stack
 * 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
 * return does not signify an error, and JS_RestoreFrameChain handles a null
 * frame pointer argument safely.
 */

So I don't have a way to signal OOM if we were to implement this with a vector in cx.

Comment 4

[Brendan Eich [:brendan]]

Duh, I see! We should change the API, it's new enough we can get away with it (did it even release in an extant spidermonkey tarball?).

Two options:

1. Change the Save API to return JSBool with a maybe-null JSStackFrame pointer in an out param.

2. (Ugly): change the API to return a non-null invalid value ((JSStackFrame *)NULL + 1) to mean error.

/be

Comment 5

[Andreas Gal :gal]

Why do we return the JSStackFrame pointer at all? Can we just make it bool?

Comment 8

[Brendan Eich [:brendan]]

The return value is for sanity checking -- we want caller to match internal set-aside, to catch unbalanced/mismatched save/restore control flow, etc. This is intentional but perhaps it's not worth the hassle.

/be

Comment 9

[Damon Sicore (:damons)]

Not a blocker.  Enhancement.  Makes recursion faster.  Still should do.

Comment 10

[Jeff Walden [:Waldo]]

Somebody fixed this sometime.