1445812 - [jsdbg2] Debugger.Frame .arguments.length is wrong in generators

Status: NEW

(Core :: JavaScript Engine, defect, P5)

Description

[Jason Orendorff [:jorendorff]]

let g = newGlobal();
g.eval('function* f() { debugger; }');
let dbg = new Debugger(g);
dbg.onDebuggerStatement = frame => {
    assertEq(frame.arguments.length, 3);
};
g.f(1, 2, 3).next();

----

InterpreterFrame::nactuals_ is always initialized to 0 when resuming a generator, so after the initial yield, frame.arguments will be empty.

Comment 1

[Jason Orendorff [:jorendorff]]

I don't see a way to fix this without penalizing all generators. P5.

Comment 2

[Jason Orendorff [:jorendorff]]

To elaborate on comment 1: In SpiderMonkey, when you call a JS function, whether from JS, C++, or jitcode, you pass CallArgs: effectively an argc and a reference to an argv. The lifetime of the reference is that call. If the callee is a generator-function, a GeneratorObject is returned to you, and you may then dismantle the argv and move on. The generator thus can't keep a reference past the initial yield point--it'd be a dangling pointer. Short of always copying the argv *just in case* the debugger needs it later, we aren't going to have it.

Jim pointed out that the documentation should mention this issue, since it's unlikely to go away.

Comment 3

[Logan Smyth [:loganfsmyth]]

Just noticed this myself. I wonder if this negates the usefulness of frame.arguments? Perhaps we should remove it entirely. I'm not sure we actually use it in the debugger, since our scope pane is only interested in named bindings, we don't really have a need for the full argument set passed to the debugger, unless the user's code actively uses the arguments, in which case things would already be accessible there.

Comment 4

[Logan Smyth [:loganfsmyth]]

Alternatively, if we leave it in, it should explicitly return null when accessed anywhere other than the first execution of a frame, and it should have its frame slot cleared so accessing properties on it throws after that point.

Currently the DebuggerArguments object is cached the first time it is looked up, so we also get this weird behavior.

var g = newGlobal({ newCompartment: true });
var dbg = Debugger(g);
g.eval(`
async function f(a, b, c) {
  await Promise.resolve();
}
`);

dbg.onEnterFrame = function(frame) {
  print(frame.arguments.length); // 4
  print(frame.arguments[0]); // 1

  dbg.onEnterFrame = function(frame) {
    print(frame.arguments.length); // 4
    print(frame.arguments[0]); // undefined
  };
};

g.f(1, 2, 3, 4);

because the object is created once when the arguments were available, and then it is never destroyed on suspend and the length was set the first time it was accessed.

Comment 5

[Logan Smyth [:loganfsmyth]]

Jim, what do you think?

Comment 6

[Jim Blandy :jimb]

There is the weird ARGS_OBJ_SLOT on AbstractGeneratorObjects:
https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/js/src/vm/GeneratorObject.h#32
Does that hold anything useful?

The next question would be, what does the engine itself do with arguments references in a generator body?

I'm fine with taking the feature if it becomes difficult to support, but I think we can invest the time to get a better picture of what's going on before we make that call.

Comment 7

[Logan Smyth [:loganfsmyth]]

Does that hold anything useful?
The next question would be, what does the engine itself do with arguments references in a generator body?

It looks like that slot is only populated if the engine decides through static analysis that the arguments object will potentially be needed. That means there will be plenty of times where we pause, and we legitimately don't know what arguments were passed to the frame. I think that make sense because otherwise SpiderMonkey would be risking potential larger memory usage due to generators keeping all of their arguments live when they don't actually use them or have a way to access them.

I'm fine with leaving this feature in if we think it is useful, but I think Jason is right that we just don't have a way to consistently access the arguments after the initial execution of the frame. We should at least explicitly return null if you use it on a suspended frame or on a resumed on-stack frame. That would mean you can only access frame.arguments between the first onEnterFrame and onPop. I think the question is whether that is enough to make the feature worth having. Looking at the codebase, I only see a single usage of it, and its one that can be easily implemented another way, so that is not a blocker. It depends on whether we think it is useful from the standpoint of the Debugger JS API for other potential usecases, but as we've discussed, it's not easy to say whether there are other consumers of this API.