Closed Bug 613319 Opened 14 years ago Closed 12 years ago

move gcRootsHash and gcLocksHash into compartment

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gwagner, Assigned: igor)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

Needed for per-compartment GC
nsAutoGCRoot uses the RT version of the API, and its obsolete. I will rip it out.
blocking2.0: --- → ?
(In reply to comment #1)
> nsAutoGCRoot uses the RT version of the API, and its obsolete. I will rip it
> out.

Ideally in FF we should stop using anything that relies on gcRootsHash and gcLocksHash. It is easy to create cycles that GC or CC would not be able to break. I.e. one can pin an object that references the structure that is supposed to unpin it. Another problem is that in the per-compartment world the semantics of the API changes substantially due to interaction with compartment lifetime. 

So my suggestion would be to consider to remove this API. The conservative GC, private slots and custom GC-marking (that one can use to build own gcRootsHash if necessary) should take care about the rest.
(In reply to comment #2)
> So my suggestion would be to consider to remove this API. The conservative GC,
> private slots and custom GC-marking (that one can use to build own gcRootsHash
> if necessary) should take care about the rest.

I.e. the plan can be like:

1. Remove the usage of the API from the browser.
2. Remove the API while giving an example how to build own root hashes using JS_SetExtraGCRoots.
(In reply to comment #3)
> (In reply to comment #2)
> > So my suggestion would be to consider to remove this API. The conservative GC,
> > private slots and custom GC-marking (that one can use to build own gcRootsHash
> > if necessary) should take care about the rest.
> 
> I.e. the plan can be like:
> 
> 1. Remove the usage of the API from the browser.
> 2. Remove the API while giving an example how to build own root hashes using
> JS_SetExtraGCRoots.

That sounds great. Igor do you want to take this?
Assignee: general → igor
Attached patch remove all uses of nsAutoGCRoot (obsolete) — Splinter Review
I had this in my queue. Igor, use it if you like.
blocking2.0: ? → final+
I do not see how this bug is needed for the compartment GC. The compartment GC can simply enumerate the table and mark only the entries that belongs to a particular compartment. So while removal of the explicit roots helps, it does not make the compartment GC impossible.

Another observation is that since these API can be implemented on top of JS_SetExtraGCRoots we need to decide what to do about it in FF. Its implementation in FF, http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcjsruntime.cpp#323, is rathe heavy. I suppose we have to compartmentalize that first. After doing that it should be clear what kind of API FF needs and then we should consider the replacements.

Still removal of unnecessary AddRoots calls cannot hurt so lets do it in separated bugs.
We should do this. nsAutoGCRoot is simply totally unnecessary at this point. Patch in bug. Doesn't have to block.
blocking2.0: final+ → ---
Blocks: 616927
No longer blocks: compartmentGC
Comment on attachment 492558 [details] [diff] [review]
remove all uses of nsAutoGCRoot

The patch was incorporated into the patch for the bug 614578
Attachment #492558 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: