1599226 - Allow enabling Ion on Chrome content (including Extensions) but disabling it on web content

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr]]

After Bug 1576254 landed; Tor tested the performance of HTTPS Everywhere and found that with Ion disabled but WASM enabled, the extension was still un-usuably slow. So the thought is to apply the same type of exception for the general JIT Compiler so that it will work for privileged code (chrome and extension code.)

javascript.options.ion is currently a process-wide boolean; but looking through where it is checked, I think we have a JSContext in all of those places; so it seems like it would be possible to move the boolean to ContextOptions like wasmForTrustedPrinciples_...

I can start prototyping this if people think that is a good approach.

Comment 1

[Luke Wagner [:luke]]

Using a symmetric approach sounds like a generally good idea.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories/cve's]) from comment #0)

javascript.options.ion is currently a process-wide boolean; but looking through where it is checked, I think we have a JSContext in all of those places; so it seems like it would be possible to move the boolean to ContextOptions like wasmForTrustedPrinciples_...

I really want to keep the JIT options process-wide because it's simpler: we don't have to worry about setting it correctly for every worker/worklet/PAC JSContext. I'm pretty sure the Wasm prefs are 'broken' in some of these cases.

Adding a process-wide ionForTrustedPrincipals bool to JitOptions should work probably? We then also have to pass a JSContext* cx to IsIonEnabled for the runningWithTrustedPrincipals() bit but that should be OK, AFAIK.

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1599226 - Allow enabling Ion selectively on addon and system principal r?luke

Comment 4

[Tom Ritter [:tjr]]

Here's a patch that looks like it should work. Seems to build/test fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a077c5d2065ccba316d365c5a7a6d5edcd70fcb

However I am not quite sure how to test it. For WASM I could do (WebAssembly in this) but I don't imagine there is a similarly easy way to test this?

Comment 5

[Tom Ritter [:tjr]]

Attachment: Bug 1599226 - Allow enabling Ion selectively on addon and system principal r?jandem

Comment 6

[Tom Ritter [:tjr]]

Attachment: Bug 1599226 - Refactor Ion.h/cpp into JitContext.h/cpp r=jandem

We need to move IsBaseLineJitEnabled into BaselineJit.h.
Then we need to include BaselineJit.h from Ion.h.

HOWEVER, lots of things include Ion.h and we create an include
loop with BaselineJit.h. Fortunately; however, the reason lots
of things include Ion.h is to get at the JitContext that's
defined in Ion.h - and it doesn't need to be.

So we move JitContext and a few other things into a separate
header and include that instead of Ion.h

Depends on D61076

Comment 7

[Tom Ritter [:tjr]]

Attachment: Bug 1599226 - Change Ion-for-Trusted-Principals into Jits-for-Trusted-Principals r=jandem

Because Tor disables both the Ion Jit and the Baseline Jit, we want
to enable both of them for trusted principals; not just Ion.

Depends on D61271

Comment 8

[Tom Ritter [:tjr]]

This is good to go, but going to wait until after the freeze

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad782d625431
Allow enabling Ion selectively on addon and system principal r=jandem
https://hg.mozilla.org/integration/autoland/rev/528083f7cf98
Refactor Ion.h/cpp into JitContext.h/cpp r=jandem
https://hg.mozilla.org/integration/autoland/rev/05b2d3c59b88
Change Ion-for-Trusted-Principals into Jits-for-Trusted-Principals r=jandem

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1599226) for causing hazards busatges CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=288245610&resultStatus=testfailed%2Cbusted%2Cexception&revision=05b2d3c59b8889d49c14951d4db324f0e681d656

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288245610&repo=autoland&lineNumber=76418

Backout: https://hg.mozilla.org/integration/autoland/rev/ecc878c4b4e0541c5bd3c676ea653744c60b4359

Comment 11

[Tom Ritter [:tjr]]

Attachment: Bug 1599226 - Suppress Hazard Analysis through nsJSPrincipals::isSystemOrAddonPrincipal r?jandem

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48254d93985b
Allow enabling Ion selectively on addon and system principal r=jandem
https://hg.mozilla.org/integration/autoland/rev/a0af6d24f388
Refactor Ion.h/cpp into JitContext.h/cpp r=jandem
https://hg.mozilla.org/integration/autoland/rev/b22c4902d295
Change Ion-for-Trusted-Principals into Jits-for-Trusted-Principals r=jandem
https://hg.mozilla.org/integration/autoland/rev/89238a8216db
Suppress Hazard Analysis through nsJSPrincipals::isSystemOrAddonPrincipal r=jandem

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/48254d93985b
https://hg.mozilla.org/mozilla-central/rev/a0af6d24f388
https://hg.mozilla.org/mozilla-central/rev/b22c4902d295
https://hg.mozilla.org/mozilla-central/rev/89238a8216db