Closed Bug 1446466 Opened 6 years ago Closed 6 years ago

Enable moz_create_arena on non-Nightly

Categories

(Core :: Memory Allocator, enhancement)

Product:
Core
Component:
Memory Allocator
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: jandem, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1446466 Crash if moz_dispose_arena is called, and comment out all callers
59 bytes, text/x-review-board-request
glandium
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 1446466 Remove Nightly-only restriction on jemalloc arena implementation
59 bytes, text/x-review-board-request
glandium
: review+
RyanVM
: approval-mozilla-beta+
Details 
tcampbell and I noticed moz_create_arena is Nightly-only:

https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/memory/build/mozjemalloc.cpp#4552-4555

Since bug 1410132 we're using this mechanism for JS memory and it would be great if this could ride the trains.
Previously, I was told Bug 1402282 and Bug 1364359 might be blockers for shipping... but they also might not be.
Flags: needinfo?(mh+mozilla)
Bug 1402282 is not a blocker per se. Bug 1364359 is one, though, but it's only a practical problem when more arenas are used and disposed of. Considering we currently only have the js arenas using the API, it's not a practical blocker. The problem is avoiding things slipping in that would cause problems. Maybe we can make moz_dispose_arena crash for now, and comment its use in js::ShutDownMallocAllocator? (assuming we don't have loops of JS_InitWithFailureDiagnostic/JS_Shutdown)
Flags: needinfo?(mh+mozilla)
I put an assert in moz_dispose_arena to confirm it is only called once. Ran on one platform; seems to be the case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc9a970c3350eb02223de9d5fea24093e7f0bd4&selectedJob=169228609

So I think the path forward for this is to do as you suggest (moz_dispose_arena crashes, comment it in ShutDownMallocAllocator) and turn it on.
I attached a slightly different proposal.

It must be fine to call moz_dispose_arena once, cause we've been doing it on Nightly for months. So I put in a Diagnostic assert to confirm we never call it twice. I was thinking we could let that bake in Nightly for a bit to confirm our suspicions, and then uplift to 60. There's no worry about the feature (again; we've had it in Nightly for months) and the diagnostic assert will only take effect in Nightly; so when we uplift to 60 the behavior will be the same as what we've had in Nightly for the past couple releases.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34683e171615d158abc2e8d630396ac69944a47d&selectedJob=169595492
Comment on attachment 8961227 [details]
Bug 1446466 Crash if moz_dispose_arena is called, and comment out all callers

https://reviewboard.mozilla.org/r/230012/#review236026

I'd rather make it always crash, and comment out the call in js/. Because doing it this way just creates a time bomb. Imagine someone adds a use of moz_create_arena and moz_dispose_arena, then we have a time bomb until moz_dispose_arena is called again, which might not even be by that new code.
Attachment #8961227 - Flags: review?(mh+mozilla)
Comment on attachment 8961228 [details]
Bug 1446466 Remove Nightly-only restriction on jemalloc arena implementation

https://reviewboard.mozilla.org/r/230014/#review236028

We'll probably want this uplifted in 60.
Attachment #8961228 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8961227 [details]
Bug 1446466 Crash if moz_dispose_arena is called, and comment out all callers

https://reviewboard.mozilla.org/r/230012/#review236862

::: commit-message-e636e:3
(Diff revision 2)
> +Bug 1446466 Crash if moz_dispose_arena is called, and comment out all callers r?glandium
> +
> +Bug 1364359 is to fix a leaked thread-local arena. Until that is fixed; it

s/thread-local//

::: memory/gtest/TestJemalloc.cpp
(Diff revision 2)
>    }
>  
>    jemalloc_thread_local_arena(false);
>  }
>  
> -#ifdef NIGHTLY_BUILD

This removal should go in the other patch. (same for the other #ifdef NIGHTLY_BUILD removed in this patch)
Attachment #8961227 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → tom
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/24cb7dfe0339
Crash if moz_dispose_arena is called, and comment out all callers r=glandium
https://hg.mozilla.org/integration/autoland/rev/e7905dcd7f1c
Remove Nightly-only restriction on jemalloc arena implementation r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24cb7dfe0339
https://hg.mozilla.org/mozilla-central/rev/e7905dcd7f1c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Tom, thanks for fixing this. Should we consider uplifting this to beta/ESR 60?
(In reply to Jan de Mooij [:jandem] from comment #18)
> Tom, thanks for fixing this. Should we consider uplifting this to beta/ESR
> 60?

Yes, for sure. I was going to let it bake on Nightly for just a few days to confirm the small changes I had made weren't going to cause any problems.
Attachment #8961227 - Flags: approval-mozilla-beta?
Comment on attachment 8961228 [details]
Bug 1446466 Remove Nightly-only restriction on jemalloc arena implementation

Approval Request Comment
[Feature/Bug causing the regression]: We'd like to improve security by separate JS Engine allocations.

[User impact if declined]: It will be easier to write exploits.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: It changes JS Engine allocations, so there's potential side effects (perf or otherwise) that can occur but...

[Why is the change risky/not risky?]: It's been enabled in Nightly since 58, so we think we have a good handle on it.
Attachment #8961228 - Flags: approval-mozilla-beta?
Comment on attachment 8961227 [details]
Bug 1446466 Crash if moz_dispose_arena is called, and comment out all callers

Important security mitigation we want in place for ESR60 which has been baking on Nightly for a long time now. Approved for 60.0b9.
Attachment #8961227 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961228 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Tom Ritter [:tjr] from comment #20)
> [Is this code covered by automated tests?]: Yes
> 
> [Has the fix been verified in Nightly?]: Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Tom's assessment on manual testing needs and the fact that this fix has automated tests.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: