652000 - JS debugger needs a way to recover calls to JSNatives from script

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JSNatives have no JSStackFrame. The debugger (and Error.stack for that matter) should show them, so we need a way to recover that information.

There are quite a few cases, but consider the basic case where a script actually explicitly, directly calls a native Function via JSOP_CALL.

The callee, this-value, and arguments are all on the stack at the moment the JSNative is first called. But the JSNative may then overwrite all those slots. Overwriting the callee slot--the one the debugger cares about the most--is in fact mandatory, since that's the return slot.

It seems like either

  * we change the JSNative contract to ban writing to vp[0] until
    after the native is done calling back into the engine; OR

  * we accept overhead, a write instruction to store the native callee
    redundantly somewhere else on the stack; OR

  * we accept that the stack can be wrong.

Comment 1

[Luke Wagner [:luke]]

This question has come up several times in the last year of stack refactoring.  There have been several cases where "oh, but what if we were called by a native" has been a confounding case for our analysis.  For example, when constructor natives lost their stack frames, we got lucky that XPCCallContexts essentially remembers the native callee which allowed us to hack up nsXPConnect::GetCaller to make nsContentUtils::GetDocumentFromCaller work again.

Option 2 seems simplest (well, after Option 1).  However, when Waldo tried to solve a similar problem in bug 631135 with a technique that didn't even add any overhead to natives-called-from-jit or property-access-from-jit, still Dromaeo regressed.  So that makes me think we'd really feel it.

As for Option 1: I dislike vp[0] clobbering and I have had bugs where I rearranged some lines of code but failed to notice that I was reading a callee after an early rval-setting.  I have tried to remove uses of this in SM as I see them (most obviated by stack scanning).  In fact, CallArgs now asserts CallArgs::rval() doesn't precede CallArgs::callee().  So it would be cool to officially ban this.

The only technicality is that pushInvokeArgs/pushInvokeFrame can pile up multiple native calls on top of a single StackFrame so, to really catch 'em all, we'd need more than just regs.sp/pc snooping; StackSpace would have to do a bit of extra record-keeping, but this shouldn't be too hard or expensive.

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to comment #1)

Thanks for the thorough response.

> Option 2 seems simplest (well, after Option 1).

Hey, option 3 is simplest of all! ;)

> The only technicality is that pushInvokeArgs/pushInvokeFrame can pile up
> multiple native calls on top of a single StackFrame so, to really catch 'em
> all, we'd need more than just regs.sp/pc snooping; StackSpace would have to do
> a bit of extra record-keeping, but this shouldn't be too hard or expensive.

I was hoping you'd say this! I had come to the same conclusion. Script-to-native calls are surely the hottest case. They're almost all leaf calls.

Sounds like we should shoot for option 1. I'll look into it.

Comment 3

[Luke Wagner [:luke]]

(In reply to comment #2)
> > Option 2 seems simplest (well, after Option 1).
> 
> Hey, option 3 is simplest of all! ;)

Oops, joke fail, I meant to say option 3...

> Sounds like we should shoot for option 1. I'll look into it.

Sweet, three birds, one bug!  If you handle the vp[0] clobbering (which is what I hope you mean by "I'll look into it" :), I'll write CalleeIter.

Comment 4

[Jeff Walden [:Waldo]]

I too am in favor of option 1, and had been thinking about possibly implementing it at some point when I found the time.

Comment 5

[Jason Orendorff [:jorendorff]]

OK, narrowing this bug to be about calls to JSNatives from script.

Filed bug 656462 for calls to JSNatives from everywhere else.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.