Last Comment Bug 613319 - move gcRootsHash and gcLocksHash into compartment
: move gcRootsHash and gcLocksHash into compartment
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 616927
  Show dependency treegraph
 
Reported: 2010-11-18 13:34 PST by Gregor Wagner [:gwagner]
Modified: 2012-02-06 21:58 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove all uses of nsAutoGCRoot (19.42 KB, patch)
2010-11-22 19:14 PST, Andreas Gal :gal
no flags Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2010-11-18 13:34:03 PST
Needed for per-compartment GC
Comment 1 Andreas Gal :gal 2010-11-18 13:46:21 PST
nsAutoGCRoot uses the RT version of the API, and its obsolete. I will rip it out.
Comment 2 Igor Bukanov 2010-11-19 01:26:18 PST
(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.
Comment 3 Igor Bukanov 2010-11-19 01:33:48 PST
(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.
Comment 4 Gregor Wagner [:gwagner] 2010-11-19 10:45:55 PST
(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?
Comment 5 Andreas Gal :gal 2010-11-22 19:14:40 PST
Created attachment 492558 [details] [diff] [review]
remove all uses of nsAutoGCRoot
Comment 6 Andreas Gal :gal 2010-11-22 19:15:18 PST
I had this in my queue. Igor, use it if you like.
Comment 7 Igor Bukanov 2010-11-24 07:40:40 PST
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 Andreas Gal :gal 2010-12-05 23:01:47 PST
We should do this. nsAutoGCRoot is simply totally unnecessary at this point. Patch in bug. Doesn't have to block.
Comment 9 Igor Bukanov 2010-12-14 12:53:57 PST
Comment on attachment 492558 [details] [diff] [review]
remove all uses of nsAutoGCRoot

The patch was incorporated into the patch for the bug 614578

Note You need to log in before you can comment on or make changes to this bug.