578154 - JM: EvalInFrame needs to recompile methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

I am going to break bug 552248's test case in the fix for bug 573083, since the method JIT assumes a frame not poisoned by eval can be optimized. We should recompile methods when this case occurs.

Comment 1

[Andrew Drake [:adrake]]

Since evalinframe basically happens at every debug hook, does it make sense to just set the eval poison bit whenever we turn on a debug feature?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #1)

Yeah, I think so.

Comment 3

[Andrew Drake [:adrake]]

Currently if the eval bit is forced on, about 100 trace tests stop working. This pretty much blocks this bug, so we're going to have to investigate that.

Comment 4

[Andrew Drake [:adrake]]

Attachment: Patch v0 (WIP)

Mostly complete WIP, missing some documentation, some API semantics of debug mode, and test cases.

Debug mode needs to be turned on for a script before it can be re-JITed for now, due to the complexity of fixing up the stack when this isn't the case. Additionally, we force the eval bit on so EvalInFrame can produce meaningful results. This patch adds two public APIs to manipulate the current debug mode, and one friend API to do so unsafely (for the shell). Additionally, semantics of many debug API functions have changed to require debug mode to be enabled. Debug mode can only safely be turned on when no scripts are executing (unless the API user *cough*shell*cough* promises to be careful). Per discussions with jimb, this would probably be most easily implemented by an event handler in the browser event loop. JSD will have to change to allow such an event to be sent, but this may be doable without altering the API. In this case, modifications to JSD alone would allow Firebug to work with no changes! I have not looked at JSD's API in any depth.

I'd appreciate any and all feedback!

Comment 5

[Andrew Drake [:adrake]]

Attachment: Patch v1

Modulo some documentation, I think this patch is ready to go. I added 6 new tests that check odd cases like EvalInFrame from within getters/setters or traps, since these can have preserved type information if debug mode is not enabled.

Additionally, this patch reincorporates testBug555248 since it passes again.

Comment 6

[Andrew Drake [:adrake]]

Also, to solve the API semantic question: it was suggested that a possible solution would be to make it easy to add or remove assertions, exception throwing, etc, so I created a function CheckDebugMode with some comments suggesting possible implementations.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 462726 [details] [diff] [review]
Patch v1

Looks great, got a few comments.

>+#ifdef JS_METHODJIT
>+    js::ScriptSet scripts;
>+#endif

This probably wants a comment describing why it's needed. It'd be nice if we could get rid of the hash set in JSThreadData now, but that can come later.

>+static JSBool
>+IsScriptLive(JSContext *cx, JSScript *script)
>+{

Internal functions like this should be C++y, s/JSBool/bool/, s/JS_TRUE/true/ etc.

>+    /* debugMode */
>+    false,

Thanks for commenting this one. Every time I have to modify that singleton my head explodes.

>+#ifdef JS_METHODJIT
>+    if (!cx->compartment->addScript(script)) {
>+        cx->free(script);
>+        return NULL;
>+    }
>+#endif

Would it make more sense to only do this after a successful method JIT?

> static JSBool
>+SetDebug(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    if (argc == 0 || !JSVAL_IS_BOOLEAN(argv[0]))
>+        return JS_FALSE;
>+    
>+    js_SetDebugMode(cx, JSVAL_TO_BOOLEAN(argv[0]));
>+    *rval = JSVAL_VOID;
>+    return JS_TRUE;
>+}

JS_FALSE will propagate a silent failure. You either want to set the rval or throw an exception.

>+static JSBool
>+CheckDebugMode(JSContext *cx)
>+{
>+    JSBool debugMode = JS_GetDebugMode(cx);
>+    JS_ASSERT(debugMode);
>+    /*
>+     * if (debugAssertionsEnabled && !debugMode)
>+     *     killSelfForCrashReport();
>+     */
>+    /*
>+     * cx->throwing = JS_TRUE; 
>+     * cx->exception = blah;
>+     */
>+    return debugMode;
>+}

All of the callers propagate up a false return from this function. So, same problem. What's the intent?

Comment 8

[Andrew Drake [:adrake]]

> All of the callers propagate up a false return from this function. So, same
> problem. What's the intent?

Good question. The possibilities look something like this:

(Assert|Check) debug mode at (recompile|api call) time, and when assertions are disabled (propogate failure|crash for backtrace).

Right now I do: Assert debug mode at api call time and when assertions are disabled propogate failure. + a double check assertion in recompile that shouldn't go away.

Comment 9

[Andrew Drake [:adrake]]

Attachment: Patch v1 -> v2

Fixes above comments. Also combines the thread data picScripts and compartment scripts sets to just be a single set of all JIT'd scripts in the compartment.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 463098 [details] [diff] [review]
Patch v1 -> v2


> JSContext::purge()
> {
>     FreeOldArenas(runtime, &regexpPool);
>+    /* :XXX: Is this the right place for this? */
>+    compartment->purge(this);
> }

Let's get Jason or Andreas to comment here.

>+     * :TODO:
>+     * This probably should be an assertion, since it's indicative of a severe
>+     * API misuse.

I don't have a strong opinion here. Both error & assert sound good.

One non-nit: ScriptSet uses a SystemAllocPolicy so any failures from that ScriptSet need an error propagated up, via js_ReportOutOfMemory. Some sort of helper function might be best. I'm not sure what needs to be done about Compartment::init() failures though.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Jason or Andreas, any compartment advice for comment #10?

Comment 12

[Andrew Drake [:adrake]]

Fixed the non-nit. Leaving the assertion comment until debugging is complete enough that a more informed decision can be made. Filing a follow up bug for compartment->purge stuff.

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/a661b26a83a1

Comment 13

[David Mandelin [:dmandelin]]

Backed out due to tinderbox failures:

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/18852a407459

Apparently this change touches a lot of stuff. Be sure to run crashtest and jstestbrowser before checking it in again.

Comment 14

[Andrew Drake [:adrake]]

This patch adds a compartment-wide list of all scripts so a debug mode switch can recompile. However, scripts can be destroyed from a compartment other than the one they were initialized in, making it impossible to update the correct script list. As a result, when a GC occurs in chrome, scripts will be left in the content compartment script list after being freed, resulting in a crash in the next content compartment GC.

There are a couple of ways to mitigate this before compartmentalized GC lands -- would it make sense to give scripts a "what compartment initialized me" field? To scan contexts like the GC does to find the compartment with us in its script list? Is something else even better?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 463098 [details] [diff] [review]
Patch v1 -> v2

># HG changeset patch
># Parent ccd63c404440ad6899b21ff9ae49e3e3c8b333c0
>
>diff --git a/js/src/js.msg b/js/src/js.msg
>--- a/js/src/js.msg
>+++ b/js/src/js.msg
>@@ -330,3 +330,4 @@ MSG_DEF(JSMSG_CSP_BLOCKED_FUNCTION,   24
> MSG_DEF(JSMSG_BAD_GET_SET_FIELD,      248, 1, JSEXN_TYPEERR, "property descriptor's {0} field is neither undefined nor a function")
> MSG_DEF(JSMSG_BAD_PROXY_FIX,          249, 0, JSEXN_TYPEERR, "proxy was fixed while executing the handler")
> MSG_DEF(JSMSG_INVALID_EVAL_SCOPE_ARG, 250, 0, JSEXN_EVALERR, "invalid eval scope argument")
>+MSG_DEF(JSMSG_NEED_DEBUG_MODE,        251, 0, JSEXN_ERR, "function can only be called in debug mode")

Misplaced "only" -- you want "function can be called only in debug mode".

>@@ -2287,9 +2284,10 @@ void
> JSContext::purge()
> {
>     FreeOldArenas(runtime, &regexpPool);
>+    /* :XXX: Is this the right place for this? */
>+    compartment->purge(this);

Did someone r+ code with :XXX: comments? Need certainty, even if it is that this is the wrong place but you can't fix it in the right one yet, in which case FIXME: bug NNNNNN.

>@@ -1309,8 +1301,13 @@ struct JSCompartment {
>     bool marked;
>     js::WrapperMap crossCompartmentWrappers;
>     bool debugMode;
>+
> #ifdef JS_METHODJIT
>-    js::ScriptSet scripts;
>+    /* Executable allocator for PIC buffers. */
>+    JSC::ExecutableAllocator *execPool;
>+
>+    /* Needed to re-JIT scripts for debug mode and so we can flush PICs. */
>+    js::ScriptSet jitScripts;

If a JSScript can be in only one compartment at a time, an alternative to the hash-set is to make intrinsic double linkage (see JSCList) as a base struct of JSScript and use a doubly-linked list (circular, but not a problem even though not needed).

This way you can remove a script from its compartment without having your hands on that compartment, either via cx or the script.

/be

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to comment #14)
> This patch adds a compartment-wide list of all scripts so a debug mode switch
> can recompile. However, scripts can be destroyed from a compartment other than
> the one they were initialized in, making it impossible to update the correct
> script list. As a result, when a GC occurs in chrome, scripts will be left in
> the content compartment script list after being freed, resulting in a crash in
> the next content compartment GC.
> 
> There are a couple of ways to mitigate this before compartmentalized GC lands
> -- would it make sense to give scripts a "what compartment initialized me"
> field? To scan contexts like the GC does to find the compartment with us in its
> script list? Is something else even better?

This is the topic of bug 584789.

Bug 584789 comment 8 has my current thinking on that. I think every JSScript should be permanently tied to a single compartment, which would make all this a non-issue.

Comment 17

[Andrew Drake [:adrake]]

This is primarily trying to figure out the best workaround until that lands. If that's going to land tomorrow, it can wait, but this patch blocks all remaining debug API functionality.

If you don't have any objections, the implicit linkage solution seems like it solves the problem now while still remaining relevant when scripts are actually tied to one compartment. Thoughts?

Comment 18

[Jason Orendorff [:jorendorff]]

Yes, I think it makes sense to bake the compartment-of-origin into every script, and it is a step toward bug 584789 (which isn't going to land tomorrow).

Comment 19

[Andrew Drake [:adrake]]

http://hg.mozilla.org/projects/jaegermonkey/rev/c644d72c8f09

Comment 20

[Brendan Eich [:brendan]]

Couldn't subclass JSScript : public JSCList? Just curious if there are C clients of JSScript, or something else precluded this.

/be

Comment 21

[Andrew Drake [:adrake]]

The emptyScriptConst is initialized statically, which you can't do (at least not without C++1x) if you inherit anything. Normally I'd just make it not statically initialized, but when compartmentalized scripts land the emptyScriptConst is either going away entirely or being replaced by a per-compartment one. In either case, this static initialization will be gone and we can turn it into a substruct.

That should be tagged with bug 586181.