977810 - Poison Baseline/Ion code in release builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Chris Peterson [:cpeterson]]

Ion code is only poisoned in debug builds. Consider poisoning in release builds or #ifdef JS_CRASH_DIAGNOSTICS (Nightly and Aurora).

https://hg.mozilla.org/mozilla-central/file/4cfb6c61b137/js/src/jit/Ion.cpp#l676

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Note that JS_POISON is already #ifdef JS_CRASH_DIAGNOSTICS-only, so we shouldn't need any special checking for nightly/aurora.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: poison-ion-code.patch

Poison freed Ion code in nightly builds.

As of bug 860254, jemalloc now poisons freed memory in both debug and release builds. bsmedberg suggested [1] poisoning freed JIT code, too. I replaced with #ifdef DEBUG with #ifdef JS_CRASH_DIAGNOSTICS, so it only affects Nightly and Aurora release builds. How much would this poison affect JS performance if it was enabled in release builds on all channels?

[1] https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/dUlUxYsFz5Q

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: poison-ion-code.patch

Also null out JitCode's code_ and pool_ pointers.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8388162 [details] [diff] [review]
poison-ion-code.patch

Review of attachment 8388162 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

::: js/src/jit/Ion.cpp
@@ +690,5 @@
> +        // Horrible hack: if we are using perf integration, we don't
> +        // want to reuse code addresses, so we just leak the memory instead.
> +        if (!PerfEnabled()) {
> +            pool_->release();
> +        }

Nit: no {}

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> How much would this poison affect
> JS performance if it was enabled in release builds on all channels?

I just tested this on Octane in the shell, and I don't see a slowdown if I enable the poisoning in an opt build. So I'm fine with removing "#ifdef JS_CRASH_DIAGNOSTICS", as long as you keep an eye on AWFY/Talos after it lands.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> I just tested this on Octane in the shell, and I don't see a slowdown if I
> enable the poisoning in an opt build. So I'm fine with removing "#ifdef
> JS_CRASH_DIAGNOSTICS", as long as you keep an eye on AWFY/Talos after it
> lands.

JS_POISON itself is #ifdef JS_CRASH_DIAGNOSTICS so that JS_POISON call would need to be replaced with a memset().

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson (:cpeterson) from comment #6)
> JS_POISON itself is #ifdef JS_CRASH_DIAGNOSTICS so that JS_POISON call would
> need to be replaced with a memset().

Ah, indeed. I have to run, but I just tried again with a memset there and perf still looks fine in the shell.

Comment 8

[Benjamin Smedberg]

Or we could (should!) define the real JS_POISON in all builds... can we try that?

Comment 9

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9984a46fc938

Comment 10

[Chris Peterson [:cpeterson]]

I filed a new bug 981991 to continue the investigation of defining JS_POISON in release builds.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9984a46fc938