1608102 - Consider turning off Ion in the parent process

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Description

[Jan de Mooij [:jandem]]

We've had some exploits abuse the ability to run arbitrary JS in the parent process (via PAC scripts, chrome JS bugs). Disabling Ion in the parent process would help mitigate that and reduce attack surface.

The main concern is devtools performance. I see a 6-9% regression on damp [0], but it's not very clear if this would translate in actual slowdowns because damp is a stress test probably benefiting more from Ion. In any case, a small regression there might be worth taking for now while we work on Ion security.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=a4302d289eaee41dda9f3a3ba101a2d1b0b7db98

Comment 1

[Ted Campbell [:tcampbell]]

The concern with devtools will be specific actions being slow. Things that come to mind are text-search. If we can figure out what these hot spots are, we can evaluate user impact. In the long run, things like text-search may be better suited to something like WASM, but I understand that a change like that cannot be done immediately.

What are the hotspots we should be investigating?

Comment 2

[Alexandre Poirot [:ochameau]]

(from the mail thread) Is there any devtools JS code with tight loops that's particularly performance critical?

Unfortunately, I don't think we have much processing code. When we do have extensive computation to do, we tend ot use workers (like source maps).
In the parent process, we mostly run UI/Frontend code which consist in a lot of Redux, React and layout.
Layout and DOM rendering tends to be our most consuming computations, followed by GC for the JS objects and then random JS code.

I imagine that Ion may have an impact of React and Redux codebases and I'm not sure there is anything to workaround here.

I'll try to investigate DAMP results to see if somethings stands out, but at first sight, the regression is real and probably significant against complex websites.

Comment 3

[:Gijs (he/him)]

How fine-grained can we do this? And do we envisage doing this permanently or just until we're more confident about Ion security? I can't quite tell from comment #0. :-)

I'd be a little concerned about how much of frontend JS hotspots are covered by talos performance tests in terms of using them to evaluate whether there'd be an impact from doing this.

Comment 4

[Alexandre Poirot [:ochameau]]

Here is a DAMP comparison for linux.
It highlights significant regressions.

• a few around the debugger. Like 34% on project search, 20 to 30% around step actions and 27% when reloading the page.
• reloading the page with the netmonitor regresses by 19%
• same thing for the console, 10%
• inspecting DOM elements and JS object is also slower respectively by 6% and 22%

The most concerning one for me is the 27% regression when reloading the page with the debugger.
I focused on just that. Here is a profile diff which mostly highlight CodeMirror code.
Unfortunately CodeMirror is compressed in m-c and so it is hard to know what particular function are being made slower.

But there is this _getDataFromTimedChannel function, used by the netmonitor function.
It is reported to be slower without Ion and it uses reduce.
Is that a typical code which is made faster thanks to Ion?

(I'm adding Julian in the loop as he is working on DAMP these days)

Comment 5

[Julian Descottes [:jdescottes]]

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

The main concern is devtools performance. I see a 6-9% regression on damp [0], but it's not very clear if this would translate in actual slowdowns because damp is a stress test probably benefiting more from Ion.

Just want to double on what Alex said, while some talos tests might be stress-tests/microbenchmarks, most of the tests in DAMP are very close to what a user will experience. For instance the subtests starting with "complicated.*": they are basically opening and reloading devtools on a website. The website is a snapshot of bild.de, which is a big website but not huge. So, the regression will most likely impact users.

Comment 6

[Ted Campbell [:tcampbell]]

This would be permanent. An replacement/retrofit for Ion is being worked on that will have better security and performance characteristics.

It does seem from the numbers that this might be quite problematic for devtools. We will probably focus on Bug 1607494 for the immediate future, and hold off on this parent-process-wide change for now.

Comment 7

[Jan de Mooij [:jandem]]

Thanks for the input folks. I'll hold off on this for now, but if we're ever able to move more of devtools out of process this could be a motivation for doing that.

Comment 8

[Ted Campbell [:tcampbell]]

With the switch from IonBuilder to Warp, we've removed the disabled and removed the code that we were concerned about here. With that change, the difference in risk between the different JITs is much smaller, so selectively turning off some JITs is less relevant.

Bug 1607494 has also landed which disables all JITs for PAC scripts which was the immediate concern since they may be controlled by local network segment.