1809613 - Allow "local" variable declaration/manipulation when using eager evaluation from DebuggerObject#call

Status: NEW

(DevTools :: Console, enhancement, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1806595 we added the ability to have "eager evaluation" when calling DebuggerObject#call
The idea was to be able to use it for custom formatter hooks to make sure executing them won't have sideeffects.

The issue now is that we are bailing if the function declare "local" variables, e.g. if you have this function

function header() {
  let ret = [1];
  ret.push(2);
  return ret;
}

with headerDbgObject being the DebuggerObject constructed from a sideEffectFreeDebugger, doing headerDbgObject.call() won't return [1,2], but null, meaning the evaluation bailed.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Arai, do you know if we could detect this specific case when we onNativeCall is triggered from https://searchfox.org/mozilla-central/rev/fb9a504ca73529fa550efe488db2a012a4bf5169/js/src/debugger/Object.cpp#2353-2356 ?

// Note whether we are in an evaluation that might invoke the OnNativeCall
// hook, so that the JITs will be disabled.
AutoNoteDebuggerEvaluationWithOnNativeCallHook noteEvaluation(
    cx, dbg->observesNativeCalls() ? dbg : nullptr);

Comment 2

[Tooru Fujisawa [:arai]]

Can you provide a testcase for the bailing case?

I've used the header function in console to see if eager evaluation works,
and what I observe so far is that eager evaluation of header function there fails because of Array.prototype.push being effectful,
and the usage of local variable isn't much related.

Then, to allow this kind of function to be evaluated eagerly, we'll need to perform some kind of data flow analysis to see:

• where each value comes from
• how the value is modified inside each native function

For the header function's specific case, the Array.prototype.push inside the function call can be considered non-effectful to the outside of the function because:

• the array is newly created inside the function, and not something comes from outside of the function (such as, parameter, or closed over binding)
• Array.prototype.push modifies the array passed as this value, and nothing else is modified

Then, currently debugger doesn't perform data flow analysis.
"being effectful" is checked for each bytecode, and relation between them aren't considered.

So, with straightforward approach, what we'll need to do would be:

1. in JS engine side:
   1. implement data flow analysis in debugger (we might be able to imitate what JIT compiler does?)
   2. pass the analyzed data to onNativeCall handler
2. in devtools debugger side:
   1. annotate each native function with "what the function can modify"
   2. do not abort eager evaluation if the native function called with given parameter does not affect non-local state

but I guess we should revisit the requirement first, to see if it's really necessary to perform the above?
is the custom formatter function completely unknown and we need full analysis to see if the function is side-effect free?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Thanks for looking into this arai

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

Can you provide a testcase for the bailing case?

Sure.

1. So with devtools.custom-formatters and devtools.custom-formatters.enabled prefs set to true, open an about:blank tab
2. Open the console and evaluate the following to setup the custom formatters

 window.devtoolsFormatters = [{
     header: (obj, config) => {
       if (obj.hasOwnProperty("customFormatHeader")) {
         const ret = ["span"];
         ret.push("hello")
         return ret;
       }
       return null;
     },
   },{
     header: (obj, config) => {
       if (obj.hasOwnProperty("customFormatHeader")) {
         const ret = ["span", "hello fallback"];
         return ret;
       }
       return null;
     },
   }
 ];

3. Evaluate ({ customFormatHeader: true }) to trigger the custom formatter

The first header function in devtoolsFormatters will bail, the second one won't, so we'll get a "hello fallback" printed in the console.

I've used the header function in console to see if eager evaluation works,
and what I observe so far is that eager evaluation of header function there fails because of Array.prototype.push being effectful,
and the usage of local variable isn't much related.

Then, to allow this kind of function to be evaluated eagerly, we'll need to perform some kind of data flow analysis to see:

• where each value comes from
• how the value is modified inside each native function

For the header function's specific case, the Array.prototype.push inside the function call can be considered non-effectful to the outside of the function because:

• the array is newly created inside the function, and not something comes from outside of the function (such as, parameter, or closed over binding)
• Array.prototype.push modifies the array passed as this value, and nothing else is modified

Then, currently debugger doesn't perform data flow analysis.
"being effectful" is checked for each bytecode, and relation between them aren't considered.

So, with straightforward approach, what we'll need to do would be:

1. in JS engine side:
   1. implement data flow analysis in debugger (we might be able to imitate what JIT compiler does?)
   2. pass the analyzed data to onNativeCall handler
2. in devtools debugger side:
   1. annotate each native function with "what the function can modify"
   2. do not abort eager evaluation if the native function called with given parameter does not affect non-local state

but I guess we should revisit the requirement first, to see if it's really necessary to perform the above?
is the custom formatter function completely unknown and we need full analysis to see if the function is side-effect free?

yes, custom formatters function (aka hooks) can be set by the page and can be anything really (that's why we'd want to bail if they have side effects in the first place)

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

by the way, I mentioned custom formatters here because it's what I hit, but I was also thinking about Bug 1639648 where we would face a similar issue

Comment 5

[Tooru Fujisawa [:arai]]

Thanks. I observed the issue, and it surely comes from Array.prototype.push.

Then, just to make sure, what's the reason why the side-effect needs to be prevented by browser side, in the custom formatter case?
is there any security concern?

even if we go with the data flow analysis way, the reason there matters much because the annotation can be too complex if we really need to cover all possible cases around side-effect, such as interaction between monkey-patched built-ins called inside the native functions.

Comment 6

[Tooru Fujisawa [:arai]]

another edge case found in bug 1806598 that some functions share single native function, and we need to be careful about how to verify the identity.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

I also hit this with Array.prototype.sort and RegExp.prototype[Symbol.split]

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

Then, just to make sure, what's the reason why the side-effect needs to be prevented by browser side, in the custom formatter case?
is there any security concern?

So not really security per-se, as those custom formatter hooks are called within the content page context, and so shouldn't bring more security concern than say a console evaluation; but I was worried about obfuscation/tracking, where a page would put a custom formatter to know if DevTools are open, and do something from there.

Now, I don't know if we should care that much. The feature will be off by default, and will only be enabled if the user turn it on from the DevTools settings panel. We might add a notice/warning in the console when those formatters are being used and a quick way to turn them down.

even if we go with the data flow analysis way, the reason there matters much because the annotation can be too complex if we really need to cover all possible cases around side-effect, such as interaction between monkey-patched built-ins called inside the native functions.

If I understand correctly, this would be a complex task that could take some time to build, and this specific case might not justify spending so much time on it.
So I guess it would be easier for us to drop the "side-effect free" requirement we put on custom formatter and mitigate the risks in some other ways.

Comment 8

[Julian Descottes [:jdescottes]]

Not urgent at the moment since it doesn't break any user interaction.