553405 - TM: JS_SetTrap ignored by tracing jit

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

David observed that the tracing jit doesn't seem to do anything for JS_SetTrap.  One possibility is that an undocumented prerequisite of JS_SetTrap is that it can only be called after other jit-inhibiting changes have occurred.  In that case, perhaps we should JS_ASSERT(rt->debuggerInhibitsJIT()) at the top of JS_SetTrap and kin.  If this is not true, then, at a minimum, JS_SetTrap should bail off trace and cause a JITInhibitingHookChange.

Anyone know more?

Comment 1

[Luke Wagner [:luke]]

Debugger hooks should inhibit the tjit.

Comment 2

[Jason Orendorff [:jorendorff]]

I don't think this is really INVALID.

It's true that if you turn on a real debugger, that will set debugger hooks that inhibit the tracejit. So in the browser, we're covered. But not all debugger hooks inhibit the tracejit. Applications can call JS_SetTrap without setting any such hooks; just run the shell with -j -d and call trap().

At record time, JSOP_TRAP causes us to give up recording. So the possible hole is if a trap is set on a script while we're already executing tracejit code for that script, *and* none of the tracejit-inhibiting debug hooks are set. I think this could happen, but as luck would have it, the shell's Trap and Untrap functions coincidentally bump us off trace (by calling JS_GetScriptedCaller from GetTrapArgs). So it's not possible to trigger it from the shell.

Comment 3

[Luke Wagner [:luke]]

I guess I left out my whole reasoning: JS_SetTrap can only be called with 'debug mode' on (we throw if not) and JS_SetDebugModeForCompartment asserts that no JS is on the stack.

Now, bug 638210 (and jimb's grand vision) wants to kill debug mode.  In that case, the "if !debugMode" check could just be replaced by LeaveTrace.

Comment 4

[Luke Wagner [:luke]]

So, perhaps it should be WORKSFORME instead of INVALID.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> I guess I left out my whole reasoning: JS_SetTrap can only be called with
> 'debug mode' on (we throw if not) and JS_SetDebugModeForCompartment asserts
> that no JS is on the stack.

But again, turning debug mode on does not inhibit the tracejit.

Seriously, if GetTrapArgs were worded a little differently I think this bug would be easy to observe in the shell.

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #5)
> But again, turning debug mode on does not inhibit the tracejit.

Fantastic.  LeaveTrace, then?