716761 - Avoid calling ScriptDebugEpilogue a second time when onPop turns a return into a throw or vice versa

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

When a function returns, a Debugger.Frame instance's onPop handler can turn that return into a throw.

In JaegerMonkey-generated code, the jitted code calls stubs::ScriptDebugEpilogue, which calls js::ScriptDebugEpilogue, which runs the onPop handler. js::ScriptDebugEpilogue notices that the resumption value returned by the onPop handler is { throw: VALUE }, requesting that VALUE be thrown at that point.

stubs::ScriptDebugEpilogue calls THROW. Control enters JaegerThrowpoline, which then calls js_InternalThrow. Unfortunately, js_InternalThrow will then call js::ScriptDebugEpilogue a second time, for the same frame --- it has no way of knowing that the exception was generated by the first js::ScriptDebugEpilogue call, and thus believes it needs to inform the debugger that the throwing frame is being popped.

In practice, this is okay, because calling js::ScriptDebugEpilogue twice on the same frame happens to have no effect. However, this is an uncomfortable lack of symmetry, and in theory it would be nicer if js::ScriptDebugEpilogue could count on being called exactly once per frame.

One way to fix this would be to change the way throwing works in JM: instead of having THROW edit the stub call's continuation to go to js_InternalThrow, and then go wherever that call directs, THROW itself could call js_InternalThrow, which could edit the stub call's continuation as it pleases.

A quick audit shows that there are a few places that use THROW in a position that is not in the tail position of a stub call (GetPropCompiler::generateGetterStub). These would need cleaning up.

Comment 1

[Jim Blandy :jimb]

Similar problems occur when an onPop handler converts a throw into a return. However, that could be fixed by changing the way the forceReturn trampolines are generated: call AnyFrameEpilogue from js_InternalThrow, instead of from the trampoline, and pass a flag indicating that ScriptDebugEpilogue should not be run again.

Comment 2

[Jim Blandy :jimb]

The description of the fix in comment 0 needs an edit:

One way to fix this would be to change the way throwing works in JM. In the status quo, THROW edits the stub's continuation to return to JaegerThrowpoline. JaegerThrowpoline calls js_InternalThrow, and then either jumps into more JIT code or exits the VMFrame. Instead, THROW itself could call js_InternalThrow, which would edit the continuation as needed.

js_InternalThrow would no longer need to use extern "C" calling conventions; it could become simply js::mjit::InternalThrow.

The forceReturn trampolines may need to be written out by hand, as they would need to be usable as continuations of stub calls. It looks like the stub call generation code that we have at present generates both the pre-call and post-call code, and forceReturn trampolines need to start with post-call code. However, there would only need to be one trampoline; the Trampoline and TrampolineCompiler types could go away.

Comment 3

[Jim Blandy :jimb]

Also, JaegerThrowpoline would need to be become a JaegerExitVMFramePoline, only used in the (common) case where js_InternalThrow kicks everything into the interpreter.

Comment 4

[Jim Blandy :jimb]

All that said, this doesn't seem like a high-priority investment given that IonMonkey is on the way.