move gcRootsHash and gcLocksHash into compartment

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gwagner, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

7 years ago
Needed for per-compartment GC
(Reporter)

Updated

7 years ago
Blocks: 605662

Comment 1

7 years ago
nsAutoGCRoot uses the RT version of the API, and its obsolete. I will rip it out.
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
(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.
(Assignee)

Comment 3

7 years ago
(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.
(Reporter)

Comment 4

7 years ago
(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)

Updated

7 years ago
Assignee: general → igor

Comment 5

7 years ago
Created attachment 492558 [details] [diff] [review]
remove all uses of nsAutoGCRoot

Comment 6

7 years ago
I had this in my queue. Igor, use it if you like.

Updated

7 years ago
blocking2.0: ? → final+
(Assignee)

Comment 7

7 years ago
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.

Comment 8

7 years ago
We should do this. nsAutoGCRoot is simply totally unnecessary at this point. Patch in bug. Doesn't have to block.
blocking2.0: final+ → ---

Updated

7 years ago
Blocks: 616927

Updated

7 years ago
No longer blocks: 605662
(Assignee)

Comment 9

7 years ago
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
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.