Open Bug 1835089 Opened 1 year ago Updated 1 year ago

Trace calls to native functions

Categories

(DevTools :: Debugger, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

Details

Attachments

(1 file)

The tracer introduced in bug 1803616 only reports about javascript function being called.
But it doesn't help see the calls being made to native objects, like document.querySelector("input"). It would be handy to be able to trace all usages of native methods as well as WebCompat probably know that some particular API will be typicaly source of WebCompat troubles.
It may also provide some better context on what the JS code is doing, given that function name are often meaningless.

:jandem, hi, I wanted to sync up with you before trying to move forward with this prototype.

This feature uses the API introduced in Add Debugger.onNativeCall hook.
I saw you challenged this new API in comment 3 and I wanted to ensure this is reasonable to double down on it and use it even more.

We recently shipped yet another new live JavaScript tracer in DevTools (meta bug).
Preffed off behind devtools.debugger.features.javascript-tracing toggleable via Ctrl+Shift+5 or from debugger UI.
This currently only traces JavaScript function calls, but it would be super helpful for the WebCompat team to also log calls made to native functions, especially DOM APIs.

I experimented with reusing insideDebuggerEvaluationWithOnNativeCallHook outside of only console eager evaluation, and enable Debugger.onNativeCall for all page executions.

If I get a green light from you/your team, I'll polish this patch and probably rename this boolean.

Flags: needinfo?(jdemooij)

(In reply to Alexandre Poirot [:ochameau] from comment #2)

:jandem, hi, I wanted to sync up with you before trying to move forward with this prototype.

My main concern about using onNativeCall is that it disables the JITs so we only use the C++ interpreter. If this only affects the new tracing mode, maybe that's okay because I expect tracing to add quite a bit of performance overhead anyway, but we should make sure it doesn't slow down other debugger uses.

CC'ing arai who may have thoughts on this as well.

Flags: needinfo?(jdemooij)

(In reply to Jan de Mooij [:jandem] from comment #3)

My main concern about using onNativeCall is that it disables the JITs so we only use the C++ interpreter. If this only affects the new tracing mode, maybe that's okay because I expect tracing to add quite a bit of performance overhead anyway, but we should make sure it doesn't slow down other debugger uses.

I'd only enable this when the tracer is enabled, and may be only when we want this extra feature.
I imagine I should then only be extra careful when I tweak insideDebuggerEvaluationWithOnNativeCallHook to support this new setup.

(In reply to Alexandre Poirot [:ochameau] from comment #4)

I'd only enable this when the tracer is enabled, and may be only when we want this extra feature.
I imagine I should then only be extra careful when I tweak insideDebuggerEvaluationWithOnNativeCallHook to support this new setup.

Yeah we should make sure we discard JIT code that has already been compiled before we started tracing. It would be good to have jit-tests for these cases too, to check that we don't miss native calls.

The feature looks very interesting, and it's what I wanted to see when investigating WebCompat issue last week :)

About tracing native functions, we might need to be careful not to expose self-hosted functions, which is also treated as native-function in the context of debugger and onNativeCall.
It's not likely it becomes security-sensitive given this is devtools feature and not accessible from web content (is it correct?), but it would be simply misleading to expose the self-hosted JS internals, and it increases noise and slows down unnecessarily.

The following test logs onNativeCall when calling Array.prototype.sort:

https://searchfox.org/mozilla-central/rev/c936f47f3a629ae49a4d528d3366bf29f2d4e4a7/js/src/jit-test/tests/debug/Debugger-onNativeCall-03.js#15-16,19,21,24,26

dbg.onNativeCall = f => {
  rv.push(f.displayName);
...
gdbg.executeInGlobal(`
...
  x.sort((a, b) => {print(a)});
...
assertEqArray(rv, [
...
  "ArraySortCompare/<",

and ArraySortCompare/< mentioned above is the following anonymous function, which is implementation details not to be exposed to web dev,
and we need to filter such function out at some layer (it might already be done in the patch tho):

https://searchfox.org/mozilla-central/rev/c936f47f3a629ae49a4d528d3366bf29f2d4e4a7/js/src/builtin/Array.js#81-82

function ArraySortCompare(comparefn) {
  return function(x, y) {

Also, if the target native function covers Xray (I suppose it doesn't happen for this case, tho), we might need to take care of the difference between the original function and the Xray function. There was somewhat related issue in bug 1812540.

Then, about the slowdown, I wonder what happens when the tracer gets turned off after getting log for certain time range.
Will it re-enable JIT for the session, or does the webpage keep being slow until reload?

Thanks a lot for the feedback!

(In reply to Jan de Mooij [:jandem] from comment #5)

Yeah we should make sure we discard JIT code that has already been compiled before we started tracing. It would be good to have jit-tests for these cases too, to check that we don't miss native calls.

Any idea how I can do that? May be some existing code already does something similar somewhere?
I know about this Debugger.Object.reparse method, but I'm not sure it does the right thing, it seems to instantiate a brand new Source:
https://searchfox.org/mozilla-central/rev/a5da23c8c9c1151dcdf0ca34b3cfd0a4d0fc3b48/js/src/debugger/Source.cpp#616-639

(In reply to Tooru Fujisawa [:arai] from comment #6)

The feature looks very interesting, and it's what I wanted to see when investigating WebCompat issue last week :)

Happy to ear that! This motivates me to push this forward :)

About tracing native functions, we might need to be careful not to expose self-hosted functions, which is also treated as native-function in the context of debugger and onNativeCall.
It's not likely it becomes security-sensitive given this is devtools feature and not accessible from web content (is it correct?), but it would be simply misleading to expose the self-hosted JS internals, and it increases noise and slows down unnecessarily.

Yes this feature should NOT be accessible to the web content.
I agree with filtering out self-hosted. May be I can filter out from the onNativeCall JS callback by filtering out based on source.url != "self-hosted"?

Also, if the target native function covers Xray (I suppose it doesn't happen for this case, tho), we might need to take care of the difference between the original function and the Xray function. There was somewhat related issue in bug 1812540.

The tracer is available in the browser toolbox, but crashes quite easily, so that's not our main focus for now.
But yes, it would be nice to make it work nicely with Xrays.
Is it about simple usecase like, tracing calling document.querySelector("div") from chrome code, where document is an xray for a content page?
Or is this about more convoluted scenarios?

Then, about the slowdown, I wonder what happens when the tracer gets turned off after getting log for certain time range.
Will it re-enable JIT for the session, or does the webpage keep being slow until reload?

I have in mind to tweak insideDebuggerEvaluationWithOnNativeCallHook to support this new setup. (my current patch is just a quick prototype)
So that if we start the tracer, this will be set to the tracer's Debugger and reverted to previous value when we stop it.
And according to Jan's suggestion, I should try to dicard JIT code when enabling the tracer. I imagine I may do something as well to help re-compile in JIT when disabling the tracer?
But at very least, when disabling the tracer, we will re-allow using the JIT:
https://searchfox.org/mozilla-central/rev/a5da23c8c9c1151dcdf0ca34b3cfd0a4d0fc3b48/js/src/jit/Jit.cpp#145-149
insideDebuggerEvaluationWithOnNativeCallHook should be undefined if there is no other devtools evaluations.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: