Closed
Bug 1244909
Opened 8 years ago
Closed 8 years ago
Remove the PersistentRooted on the cx footgun
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
5.24 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
For performance reasons, we put Rooted on the JSContext instead of on the JSRuntime: one fewer derefs in a fast path. For Rooted this does not matter because the context cannot die while the Rooted is on the stack. We mirrored the same storage structure for PersistentRooted for simplicity. Unfortunately, it is quite possible for PersistentRooted to outlive any particular JSContext. This can easily lead to UAF if the PersistentRooted user is not careful. There are a few potential solutions: 1) Remove the JSContext* constructor and force all users to use JS_GetRuntime(aCx) or somesuch. 2) Automatically take cx->runtime() when passed a JSContext instead of a JSRuntime. 3) Move all PersistentRooted to the Zone so that we don't have to visit all of them when doing a compartmental collection. I just moved all [Persistent]Rooted to the JSRuntime as an experiment and did not see any slowdown on Octane. I'd like to do some more experimentation with the Zone to see what the overhead is like and if we have cross-zone issues or just cross-compartment issues.
Assignee | ||
Comment 1•8 years ago
|
||
Let's try out the new talos comparison tool! https://treeherder.mozilla.org/#/jobs?repo=try&revision=20632a5db83a https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=20632a5db83a
Assignee | ||
Comment 2•8 years ago
|
||
This is proving to be difficult for a number of semi-unrelated reasons, so let's just do this the boring way for now. I'm sad to have to duplicate the root-lists accessors, but we just need these to be different. https://treeherder.mozilla.org/#/jobs?repo=try&revision=754a950c67d1
Comment 3•8 years ago
|
||
Comment on attachment 8718462 [details] [diff] [review] 6.1_fix_the_persistentrooted_footgun-v0.diff Review of attachment 8718462 [details] [diff] [review]: ----------------------------------------------------------------- Heh. boilerplate is boilerplatey.
Attachment #8718462 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0497b57d8b67ee3bc1ad8e44aa26cb818ed80e0 Bug 1244909 - Store all persistent roots on the JSRuntime; r=sfink
Assignee | ||
Comment 5•8 years ago
|
||
The bustage in the try run was from me forgetting to update the jsapi-test. I fixed this locally. The rest was green enough that I'm confident pushing without a second try run.
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0497b57d8b6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•