Closed Bug 1244909 Opened 8 years ago Closed 8 years ago

Remove the PersistentRooted on the cx footgun

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8718462 - Flags: review?(sphink)
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+
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.
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.