342737 - JS_NewContext/JS_DestroyContext callback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently test cases that checks defence against stack overflow like in bug 324278 fails in the browser when running with jsd debugger calls JS_NewContext without corresponding JS_SetThreadStackLimit call. 

One way to fix this is to patch the debugger and all other users of JS_NewContext,  see http://lxr.mozilla.org/seamonkey/ident?i=JS_NewContext , with proper calls. But this would add a lot of itercode dependancies. Thus I suggest to add an API that would allow an embedding to register a callback that would be called when JS_NewContext is about to return. Then the embedding can trivially make sure that   JS_SetThreadStackLimit is called and the fix would touch just jsapi.h, jscntx.c and dom/src/base/nsJSEnvironment.cpp.

Comment 1

[Brendan Eich [:brendan]]

Sounds good to me so long as context is fully initialized, and failure of hook is allowed.

/be

Comment 2

[Igor Bukanov]

Attachment: Context callback API

The patch just adds new API , a real fix would be follow as a separated patch.

Comment 3

[Igor Bukanov]

Attachment: Context callback API

The patch just adds new API , a real fix would follow as a separated patch.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 227100 [details] [diff] [review]
Context callback API

>+    cxCallback = rt->cxCallback;
>+    if (cxCallback && ! cxCallback(cx, JS_NEW_CONTEXT)) {

Nit: no space after !.

>+            cxCallback(cx, JS_DESTROY_CONTEXT);

Comment in jspubtd.h about how JS_NEW_CONTEXT callback can fail, but JS_DESTROY_CONTEXT cannot.

>-typedef enum JSGCMode { JS_NO_GC, JS_MAYBE_GC, JS_FORCE_GC } JSGCMode;
>+typedef enum JSDestroyContextMode {
>+    JS_NO_GC,
>+    JS_MAYBE_GC,
>+    JS_FORCE_GC,
>+    JS_FAILED_NEW_CONTEXT
>+} JSDestroyContextMode;

Nice, but go further: enums of this sort use a prefix that compresses their tag somehow, and generally have enumerators of more similar string length. For example (from immediately following diff context):

> typedef enum JSRuntimeState {
>     JSRTS_DOWN,
>     JSRTS_LAUNCHING,
>     JSRTS_UP,
>     JSRTS_LANDING
> } JSRuntimeState;

Suggest:

typedef enum JSDestroyContextMode {
    JSDCM_NO_GC,
    JSDCM_MAYBE_GC,
    JSDCM_FORCE_GC,
    JSDCM_NEW_FAILED
} JSDestroyContextMode;

>+typedef JSBool
>+(* JS_DLL_CALLBACK JSContextCallback)(JSContext *cx, uintN contextOp);

Comment this with fallible NEW vs. infallible DESTROY rule.  Thanks,

/be

Comment 5

[Igor Bukanov]

Attachment: Context callback API v2

This is mostly to provide comments and address nits. In the patch I renamed JS_(NEW_DESTROY)_CONTEXT to JSCONTEXT_(NEW|DESTROY) to follow the pattern of  JSGCStatus and other enumerations.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 227194 [details] [diff] [review]
Context callback API v2

> struct JSRuntime {
>     /* Runtime state, synchronized by the stateChange/gcLock condvar/lock. */
>     JSRuntimeState      state;
> 
>+    JSContextCallback   cxCallback;

A one-line comment about "Context create/destroy callback." before cxCallback's declaration would be nice.

>+/*
>+ * The possible values for contextOp when the runtime calls the callback are:
>+ *   JSCONTEXT_NEW      JS_NewContext succesfully created new JSContext

"a new JSContext"

>+ *                      instance. The callback can initialize the instance as
>+ *                      required. When the callback returns false, the instance

s/When/If/

>+ *                      will be destroyed and JS_NewContext returns NULL. In

Prosaic style favors "null" not "NULL".

>+ *                      this case the callback is not called again with
>+ *                      contextOp set to JSCONTEXT_DESTROY.

Or just "is not called again."

>+ *   JSCONTEXT_DESTROY  One of JS_DestroyContext* methods is called. The
>+ *                      callback can perform its own cleanup and must always

s/can/may/

Looks great otherwise -- thanks!

/be

Comment 7

[Igor Bukanov]

Attachment: Context callback API v2b

This the patch to commit.

Comment 8

[Igor Bukanov]

I rename the to bug to reflect its true nature. I will file a separated bug to fix missed SetStackLimit problem.

Comment 9

[Igor Bukanov]

I committed the patch from comment 7 to the trunk.