651599 - The function registered by JS_SetCallHook does not get called before native functions

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Robin Ehrlich]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB6; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; OfficeLiveConnector.1.3; OfficeLivePatch.0.0)
Build Identifier: 1.8.5

The function registered by JS_SetCallHook does not seem to be called in the 
same way as in 1.7. In 1.7 the hook function is called before every native 
function is invoked. This does not seem to occur in 1.8.5. If I register a 
function via JS_SetFunctionCallback, it is called at the correct times, but 
there is no JSStackFrame paramater so I cannot get the function arguments 
for tracing / debugging. Is this a bug or a design change? If 
JS_SetFunctionCallback is to be used, could the stack frame be added as an 
argument?

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Native function calls no longer create stackframes (just like fastnative calls didn't before), which is presumably what you're seeing.

The fact that they don't create them is definitely a design change.

Comment 2

[Robin Ehrlich]

Let me be a little clearer. I don't care if a stack frame is created or not. What is needed is the ability to get the arguments that are passed to a native function so that they can be traced. I just need the "argc" and "vp" arguments passed to the hook function. Using the JS_SetFunctionCallback is fine with me.

Comment 3

[Robin Ehrlich]

Here is the signature I require of the callback function. I have implemented code in the js185 source release to implement this feature. I can provide the code if desired.

typedef void (*JSFunctionCallback)(const JSFunction *fun,
                                   const JSScript *scr,
                                   const JSContext *cx,
                                   jsbytecode *pc, uintN argc, jsval *vp,
                                   int entering);

Comment 4

[Robin Ehrlich]

After doing a little more testing, the JS_SetFunctionCallback is also not called when the native constructor is called. I think it should be called in this case also.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to comment #4)
> After doing a little more testing, the JS_SetFunctionCallback is also not
> called when the native constructor is called. I think it should be called in
> this case also.

Yes, it should. It's basically due to sheer laziness on my part that it doesn't. (I was having so many problems getting the basics working right that I punted on constructors.)

Note that JS_SetFunctionCallback() has rotted a bit, as is the fate of all things that are not part of the default build. enters/exits are no longer guaranteed to balance, at least when using the methodjit. Need to fix it, and set up a build with --enable-trace-jscalls so it'll run the tests that would've caught it.

(In reply to comment #3)
> Here is the signature I require of the callback function. I have implemented
> code in the js185 source release to implement this feature. I can provide the
> code if desired.

I'd like to see it. I removed the extra parameters because I was told they wouldn't work with one or other of the JITs. The tracejit, I think.

Comment 6

[Robin Ehrlich]

Attachment: Changes I have made to add more arguments to JS_SetFunctionCallback

Here are the changes I have made to add my desired arguments to JS_SetFunctionCallback. The changes work fine in calling native functions. For Javascript functions the arguments are not passed because I don't understand the internals of the engine well enough to make those changes. Additionally JS_SetCallHook works to provide this functionality.

Comment 7

[Wesley W. Garland]

Robin -- is that patch intended to land in the mozilla codebase?  If so, can you please re-generate it as an 8-line unified diff? e.g.

# diff -p -U 8 oldFile.c newFile.c

or similar?   (If you are using hg, there are instructions for submitting patches on developer.mozilla.org).

Once you have the patch uploaded, you need to request code review. Do this by looking for "r?" in the Bugzilla properties of your patch, and add the Bugzilla address of the JS peer to review (I suggest [e-mail])

Comment 8

[Steve Fink [:sfink] [:s:]]

I'm happy to review that one too.

Comment 9

[Robin Ehrlich]

Attachment: Updated diffs to trace constructors

The changes I have made to trace calls seems to work well. I would appreciate it if these changes could be incorporated into the next Javascript release.

Comment 10

[Wesley W. Garland]

Comment on attachment 549099 [details] [diff] [review]
Updated diffs to trace constructors

Not my patch, but setting review flag to sfink, hope that's okay! (I'd like this patch too)

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 549099 [details] [diff] [review]
Updated diffs to trace constructors

Review of attachment 549099 [details] [diff] [review]:
-----------------------------------------------------------------

This should really try to collect additional information out of the method JIT too. See stubs::ScriptDebugPrologue and stubs::ScriptProbeOnlyPrologue in js/src/methodjit/InvokeHelpers.cpp. The pc should be straightforward. Not sure about arguments. You can leave them out for now if they're hard (or impossible.)

Please flag me for review explicitly when you have an updated patch.

::: js-1.8.5.orig/js/src/jsapi-tests/testFuncCallback.cpp
@@ -20,5 @@
>  {
>      if (entering > 0) {
>          ++depth;
>          ++enters;
> -        if (! JS_ON_TRACE(cx))

This is no longer necessary.

::: js-1.8.5.orig/js/src/jsinterp.cpp
@@ +1246,5 @@
> +            Probes::enterJSFun(cx, fun, script, pc, args.argc(), args.base());
> +            JSBool ok = CallJSNativeConstructor(cx, fun->u.n.native, 
> +                                                args.argc(), args.base());
> +            Probes::exitJSFun(cx, fun, script, pc, args.argc(), args.base());
> +            return ok;

I don't think this is right. For one thing, "enterJSFun" should really only get called for Javascript scripts, and this is a native function. Probes::calloutBegin/calloutEnd (sorry for the odd terminology; it wasn't my idea) are the native function probe points, though they currently do not invoke the function callback.

I did the same (incorrect) thing in some of my patches; not sure if any of those ever made it in the tree.

Related, this is claiming that you're calling a function associated with 'script'. In particular, the callback handler is likely to refer to script->filename and script->lineno. But unless I'm mistaken, 'script' here is the *caller*, not the callee. So using 'script' is deceptive, at least when using Probes::enterJSFun. It would just result in any JS function that calls native functions getting reported as being called multiple times (once for the "real" call, then again when it calls native functions.)

I'd be fine with redefining Probes::calloutBegin to take caller information as well as whatever is available for the native callee. I'm not sure whether it should invoke the function callback with magic parameters, or (probably better) a newly created one. Yes, this disagrees with my earlier comment in this bug where I said that it should invoke the function callback. But a native function invocation callback has different information of interest than a JS invocation callback -- eg, function pointer value.

At any rate, I'd prefer that the patches and bugs for adding additional information to Probes::enterJSFun be separate from those for adding in native calls. They're very different in my mind, and I can imagine people objecting to one but not the other.

::: js-1.8.5.orig/js/src/jsprobes.h
@@ +62,5 @@
> +                           jsbytecode *pc = NULL, uintN argc = 0,
> +                           js::Value *vp = NULL, int counter = 1);
> +    static void exitJSFun(JSContext *, JSFunction *, JSScript *,
> +                          jsbytecode *pc = NULL, uintN argc = 0,
> +                          js::Value *vp = NULL, int counter = 0);

I'd rather not have any of these arguments defaulted but the last. The rest of the patch seems to explicitly pass in the defaults when the information is not available, anyway.

@@ -190,5 @@
>          JAVASCRIPT_EXECUTE_START((script->filename ? (char *)script->filename : nullName),
>                                   script->lineno);
>  #endif
>  #ifdef MOZ_TRACE_JSCALLS
> -    cx->doFunctionCallback(NULL, script, 1);

startExecution/stopExecution no longer do callbacks, so this is now unnecessary.

@@ -203,5 @@
>          JAVASCRIPT_EXECUTE_DONE((script->filename ? (char *)script->filename : nullName),
>                                  script->lineno);
>  #endif
>  #ifdef MOZ_TRACE_JSCALLS
> -    cx->doFunctionCallback(NULL, script, 0);

See above.

::: js-1.8.5.orig/js/src/jstracer.cpp
@@ +10585,2 @@
>      else
> +        Probes::exitJSFun(cx, fun, script, NULL, 0, NULL, enter);

How useful is this additional information if it's only available in certain runmodes? Specifically, it appears that you'll only get it when running in the interpreter, not if you're method jitted or trace jitted. Which means you have to either disable all JITting, or you can't rely on it existing. I suppose it's somewhat useful to disable JITting and then doing some analysis, but it seems limited. Am I interpreting (no pun intended) this correctly?

Comment 12

[Steve Fink [:sfink] [:s:]]

Ok, I finally looked at the original topic of this bug: JS_SetCallHook.

I am guessing that it stopped getting called when we stopped creating JS stack frames for native calls, because the callback is given a stack frame and one no longer exists. So that usage just isn't going to work anymore. It disappeared for the same reason that JS_SetFunctionCallback doesn't pick these things up -- both are intended for JS calls, not native calls.

So it still seems like the best way forward would be to create a new (non-debug) hook that is invoked on any native call and is given both caller and callee information.

I'm looking into this. It seems straightforward for the interpreter, and anything but for the JITs. I have it working for at least some cases now, but I need to figure out whether I have them all.

Comment 13

[Robin Ehrlich]

> So it still seems like the best way forward would be to create a new
> (non-debug) hook that is invoked on any native call and is given both caller > and callee information.

That sounds fine with me. When the next tarball is released I will upgrade our code to use this new feature. The hook needs to be called at invocation and return so that input arguments and return values can be traced.

Thanks,
Robin

Comment 14

[Steve Fink [:sfink] [:s:]]

I forked off bug 677985 for the new API.

Comment 16

[Robin Ehrlich]

I am submitting the diffs to include this feature in the mozjs17 source release. I would appreciate it if these changes could be included in the next RC. With the mozjs17 codebase, there are less changes. From the previous review of this feature request, it seems like there was agreement that this change could be made.

Comment 17

[Robin Ehrlich]

Attachment: diffs to include this feature in the mozjs17 source release

Please see the latest comment from me. I did not realize comments could be attached to attachments.

Comment 18

[Till Schneidereit [:till]]

Comment on attachment 728763 [details] [diff] [review]
diffs to include this feature in the mozjs17 source release

Review of attachment 728763 [details] [diff] [review]:
-----------------------------------------------------------------

Setting review flag so sfink is sure to see this patch.

The following comments are just the result of a quick drive-by review, and in no way meant to be complete.

::: mozjs17.release/js/src/jsapi-tests/testFuncCallback.cpp
@@ +19,4 @@
>  funcTransition(const JSFunction *,
>                 const JSScript *,
>                 const JSContext *cx,
> +					jsbytecode *pc, unsigned int argc, jsval *vp, 

Nits: too much space at the line's beginning, space at EOL, multiple args in same line.

@@ +35,4 @@
>  static JSBool called2 = false;
>  
>  static void
> +funcTransition2(const JSFunction *, const JSScript*, const JSContext*, 

Nit: space at EOL

@@ +47,4 @@
>  funcTransitionOverlay(const JSFunction *fun,
>                        const JSScript *script,
>                        const JSContext *cx,
> +							 jsbytecode *pc, unsigned int argc, jsval *vp, 

Nits: too much space at the line's beginning, space at EOL, multiple args in same line.

@@ +51,3 @@
>                        int entering)
>  {
> +	(*innerCallback)(fun, script, cx, pc, argc, vp, entering);

Nit: too much space at line's beginning

::: mozjs17.release/js/src/jsapi.h
@@ +1943,4 @@
>  (* JSFunctionCallback)(const JSFunction *fun,
>                         const JSScript *scr,
>                         const JSContext *cx,
> +                       uint8_t *pc, unsigned int argc, jsval *vp,

Nit: multiple args on same line

::: mozjs17.release/js/src/jscntxt.h
@@ +1388,4 @@
>  
>      void doFunctionCallback(const JSFunction *fun,
>                              const JSScript *scr,
> +                            jsbytecode *pc, unsigned int argc, jsval *vp,

Nit: multiple args on same line

::: mozjs17.release/js/src/jscntxtinlines.h
@@ +374,5 @@
> +    JSFunction *fp;
> +    jsbytecode *pc;
> +    FrameRegs *regs;
> +    JSScript *script;
> +    StackFrame *sfp;

You can move these declarations down to their first usage. That also removes the need to separately initialize fp, pc and script to NULL.

@@ +390,5 @@
> +                script = sfp->script();
> +        }
> +
> +        JSObject &callee = args.callee();
> +        RootedFunction fun(cx, callee.toFunction());

No need to root this function if you're only using it un-rooted, afterwards. In this case, you could just use `args.callee().toFunction()`, I guess.

@@ +392,5 @@
> +
> +        JSObject &callee = args.callee();
> +        RootedFunction fun(cx, callee.toFunction());
> +        fp = fun;
> +    

Nit: spaces in empty line

Comment 19

[Robin Ehrlich]

Would you like me to resubmit the changes with the suggestions made? I really hope that these changes can be included in the next source code release.

Comment 20

[Till Schneidereit [:till]]

(In reply to Robin Ehrlich from comment #19)
> Would you like me to resubmit the changes with the suggestions made? I
> really hope that these changes can be included in the next source code
> release.

That'd probably help, yes. I doubt sfink would argue against any of my feedback items.

Regarding the next source code release: That'll happen for FF24, the next LTS release. So, we've got until November to land this. :)

Comment 21

[Robin Ehrlich]

Attachment: resubmit of the diffs including the suggested changes

This is basically a resubmission the previous diffs with the suggested formatting changes and a couple of minor edits.

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 730778 [details] [diff] [review]
resubmit of the diffs including the suggested changes

This patch reverted back to non-unified diffs and so showed up as blank in the splinter review.

I would still prefer if native calls were given to a different callback, but this is still strictly an improvement so I'm ok with it as is.

I'm less sure how/where to land this. I kind of doubt they'd want anything like this on esr17. Is there a separate repo for js17? (I'm ok with this landing on mainline, but I don't know if that does you any good.)

Going forward, I don't know how useful this is until it gets support from the JITs. I also don't know if there's a hopeful future for things like this that add friction to JIT implementations.

Comment 23

[Robin Ehrlich]

> resubmit of the diffs including the suggested changes

> This patch reverted back to non-unified diffs and so showed up as blank in the > splinter review.

The diffs were created with the "-p" option. I thought this was the desired option. I can re-generate, if you give me the diff options to use.

Comment 24

[Steve Fink [:sfink] [:s:]]

The -u option is the important one (though -p is nice to have to.) It would be easier to generate these patches and keep them up to date with a revision control system, though. (hg or, if you're already familiar with git and don't want to learn a new one, git.) The most up to date documentation seems to be at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

But there's no need to update the patch just for that change. I read it as plain text.

Comment 25

[Tom S [:evilpie]]

JS_SetCallHook was removed.