1561424 - [meta] Eager Evaluation server support

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Jason Laster [:jlast]]

Chrome recently released an improvement for the console to support instant safe evaluations "eager evaluations", which helps users get a better idea of what their expression will do.

https://developers.google.com/web/updates/2018/05/devtools#eagerevaluation

The challenge of the feature is to define the right API that is safe so that you do not accidentally run stateful code. Chrome implements this here, with a whitelist.

Here is a link to a draft PRD

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Should we make this a dupe of Bug 1460518? Also, Bug 1557226 is not about what you're typing in the input, but about your console API calls (i.e. variable name/expressions would be displayed in the output, in the message the call produced)

Comment 3

[Jason Laster [:jlast]]

Thanks

Comment 4

[Brian Hackett [Laid off!]]

Attachment: WIP

This is a WIP which watches for side effects in evaluated scripts and their callees and aborts execution of the evaluation if one occurs. This isn't very complicated:

• We mark opcodes which are effectful: they can have side effects which are observable outside the frame which is executing the code. This allows writing to local variables, or creating objects and initializing their properties, but not writing to aliased variables (ones stored in environment objects, which can be accessed by other frames) or writing to object properties (even if the object was just created within the frame).

• When evaluating code, we set an onEnterFrame hook, and within every script that executes, we set breakpoints at all the offsets of opcodes which are effectful.

• If any of these breakpoints is hit, the evaluation is terminated, as if by the slow script dialog. (Debugger breakpoints already provide this functionality.)

This takes care of side effects from scripts, but side effects from native functions are another issue, and require some consideration. This is the behavior I think we should have:

• Native getters are never considered effectful. The devtools already make this assumption: hasSafeGetter in DevToolsUtils.js already permits getters to be called eagerly if they are backed by a native function instead of a script. The assumption here is presumably that such functions should not have script-visible side effects, even if they do have side effects in the C++ heap.

• Similarly, resolve hooks (called when figuring out if an object has a certain property), getters/enumerators/etc. on non-scripted proxies can and often will have side effects in the C++ heap, but should be considered free from script-visible side effects. Using this treatment will both allow a lot more code to be considered non-effectful, and avoid needing to make invasive changes to spidermonkey to watch for the points these natives are called.

• Native setters are always considered effectful (!).

• Native functions which are called directly are considered effectful unless they are in a whitelist.

I think the best way to implement this is by adding a Debugger.onNativeCall hook, which intercepts calls to native functions, getters, and setters. The hook would be called with the native and with the type of call being made, and returns a resumption value which can be used to terminate execution or modify the native's behavior. This shouldn't be too hard, though the JITs inline calls to native functions and need to be handled carefully.

Comment 5

[Jim Blandy :jimb]

And so the reason getters, setters, and scripted proxies are okay is that they just turn into function calls, which we're already handling with the onEnterFrame instrumentation. That's a nice approach.

What happens when a getter/setter/proxy handler is a cross-compartment reference to a non-debuggee function? The onEnterFrame hook won't fire for such a call. Are those covered by the non-scripted proxy case?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb from comment #5)

What happens when a getter/setter/proxy handler is a cross-compartment reference to a non-debuggee function? The onEnterFrame hook won't fire for such a call. Are those covered by the non-scripted proxy case?

The WIP uses dbg.addAllGlobalsAsDebuggees() so that we will catch any scripted function which executes, regardless of its compartment. So far there hasn't been a need to do anything special with proxies, and I'm hoping it can stay that way.

Comment 7

[Jim Blandy :jimb]

The WIP uses dbg.addAllGlobalsAsDebuggees() so that we will catch any scripted function which executes, regardless of its compartment.

Okay, makes sense. What sort of implementation do you have in mind for the release? Maybe an onCrossCompartmentCall hook?

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jim Blandy :jimb from comment #7)

The WIP uses dbg.addAllGlobalsAsDebuggees() so that we will catch any scripted function which executes, regardless of its compartment.

Okay, makes sense. What sort of implementation do you have in mind for the release? Maybe an onCrossCompartmentCall hook?

I'm hoping we can continue using addAllGlobalsAsDebuggees. The WIP removes the debuggees as well once it has finished with the eager evaluation, so there shouldn't be any problem with using this method. It might be cleaner if a special debugger was used for doing the eager evaluations which always debugs all globals, but that's a small detail. If there is a performance problem down the line with this strategy then we could look for other options, but I would be surprised if that happened.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #8)

If there is a performance problem down the line with this strategy then we could look for other options, but I would be surprised if that happened.

This happens synchronously when typing in the console right? Can you measure how long this takes when you have a content process with a ton of Facebook, Gmail, GDocs tabs in it? It seems to me that onEnterFrame has to deoptimize all JIT code for all scripts in the runtime so it wouldn't surprise me too much if that got slow with many compartments.

Comment 10

[Jan de Mooij [:jandem]]

Oh also, what's the effect on GC when you have many debuggees? Do these still end up in a single sweep group?

(I often see complaints about our devtools being slow, so if it were me I'd be wary of adding potential new cliffs.)

Comment 11

[Jim Blandy :jimb]

(needinfo'ing bhackett)

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] (PTO until Sep 16) from comment #9)

(In reply to Brian Hackett (:bhackett) from comment #8)

If there is a performance problem down the line with this strategy then we could look for other options, but I would be surprised if that happened.

This happens synchronously when typing in the console right? Can you measure how long this takes when you have a content process with a ton of Facebook, Gmail, GDocs tabs in it? It seems to me that onEnterFrame has to deoptimize all JIT code for all scripts in the runtime so it wouldn't surprise me too much if that got slow with many compartments.

I guess I'm more interested in standing something up so that devtools server/client work can start. New devtools features are gated behind prefs until they mature, and part of that process is making sure that performance is acceptable. If addAllGlobalsAsDebuggees() is causing performance or GC problems, we could just add the debuggees for the tab being debugged plus any running chrome/etc. code (which should be a lot smaller than a complex tab's content). A fix like that wouldn't require any new debugger hooks or significant changes to the strategy. It would mean, however, that eager evaluation could produce side effects in other tabs, but the issue of tabs interacting with each other already crops up in other areas of the devtools --- for example, other tabs can interact with a paused tab and break run-to-completion semantics in the paused tab. I would rather expand that class of problems if it means being able to move forward here, and down the line we can look into ways to fix the entire class rather than fixing parts of it piecemeal.

Comment 13

[Jim Blandy :jimb]

How badly do we need to support cross-compartment calls at all? Could we short-cut the question with a hard-coded test that just terminates cross-compartment calls?

Comment 14

[:Harald Kirschner :digitarald]

Can you help me understand the constraint – which scripts or DOM APIs would depend on cross-compartment calls?

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just setting NI so, this isn't forgotten.

Honza

Comment 16

[Jim Blandy :jimb]

As far as I know, there's nothing in an ordinary web page that would involve cross-compartment calls at all. Every window gets its own copy of the DOM objects and functions.

Comment 17

[Brian Hackett [Laid off!]]

While there might be performance issues we need to fix in the future, the spidermonkey changes a few months ago give the server everything needed to support eager evaluation.