78974 - JS_AddRoot and JS_AddNamedRoot (SpiderMonkey) engine API's don't work

Status: VERIFIED INVALID

(Core :: JavaScript Engine, defect)

Description

[Edward Chan]

This is a bug with the JavaScript C Engine (SpiderMonkey).

The JS_AddRoot and JS_AddNamedRoot API's do NOT work properly and cause a 
crash.  If I understand correctly, these API's are supposed to protect an 
object from being garbage collected.  However, it appears that when the garbage 
collector is called, and there is something in the GC's hash table (items that 
are supposed to be protected) from a call to JS_AddRoot/JS_AddNamedRoot, then 
we crash.  The current situation is that when the GC gets called, you must have 
already called JS_RemoveRoot for each call to JS_AddRoot/JS_AddNamedRoot.  But 
that defeats the purpose.

What I want is to call JS_AddRoot/JS_AddNamedRoot to protect the item(s) from 
being garbage collected, so that when the GC runs, these items are "not" GC'd.  
Then at a later time, when I am ready to have them cleaned up, I call 
JS_RemoveRoot, and the next time the GC is called, these items "are" GC'd.

In the code, when running in debug mode, there is a JS_ASSERT error on line 924 
of jsgc.c.  The line reads:

JS_ASSERT(root_points_to_gcArenaPool);

It's funny, because the error message that is printed above this line 
says, "This is usually caused by a missing call to JS_RemoveRoot."  But this is 
exactly the point, I shouldn't need to call JS_RemoveRoot until later, when I 
really want the item to get GC'd.

For now, the way to get around this seems to be to call JSVAL_LOCK and 
JSVAL_UNLOCK.  However, these API's have been deprecated.

Comment 1

[Phil Schwartau]

Ed, how recently have you pulled your JS source? Is it directly from the 
CVS repository, or from a published tarball? I'd just like to get that info 
into the bug; thanks -

Comment 2

[Phil Schwartau]

Ed replied: 

> It was retrieved from CVS back around April 2.  
>
> For the time being, I can use the deprecated JSVAL_LOCK and JSVAL_UNLOCK    
> API's, but I'd like to get up on the newest supported API's when this gets      
> resolved.  Thanks!!!

Comment 3

[Phil Schwartau]

Ed, do you have a reduced testcase that exhibits the crash? If so, please 
attach it to this bug -(See the link "Create a new attachment" above)

cc'ing Brendan in case the answer is already clear - 

Comment 4

[Edward Chan]

Attachment: cpp source file to demonstrate bug

Comment 5

[Edward Chan]

I attached a simple program (sorry, it's in C++) that demonstrates the 
problem.  Please see my comments for more info.

Comment 6

[John Bandhauer]

You are misusing the api. These functions expect to be passed the *address*
where a JSObject* (or other gcthing pointer) is stored. So you would do
something like:

  static JSObject* obj = some_object;
  JS_AddNamedRoot(cx, &obj, "my root");

Note that in your example you can not simply change the param to JS_AddNamedRoot
to be &pNewObj because that location is in temporary stack space. If you really
need to root the objects referenced by these pointers then you need to use
either static or heap allocated space to hold the pointers.

see the docs...
http://www.mozilla.org/js/spidermonkey/apidoc/sparse-frameset.html

Comment 7

[Brendan Eich [:brendan]]

Or use JS_LockGCThing and JS_UnlockGCThing, which are *not* deprecated.

/be

Comment 8

[Edward Chan]

OK, thanks.  But then the docs seem to be in error.  The doc says JS_AddRoot 
and JS_AddNamedRoot take a pointer, not a pointer to a pointer as you said.

Also, the docs say that JS_LockGCThing/JS_UnlockGCThing "are" deprecated.  Are 
the docs incorrect?

When would I use JS_AddRoot or JS_AddNamedRoot?  What's the difference between 
JS_AddRoot/JS_LockGCThing/JSVAL_LOCK, which all seem to do the same thing?

Also, what does it mean to "root" something?  And when would I use 
JS_AddNamedRoot instead of JS_AddRoot?

Sorry for all the confusion, but the docs don't seem to be too clear.  And with 
so many API's that seem to do the same thing, it is a bit confusing as to which 
one to use.  Thanks for the help!!!

Comment 9

[Brendan Eich [:brendan]]

The docs lie, I did not write or review them.  Rob Ginda and I are slowly fixing
them, when we can spare the time and where the payoff is greatest.  We'll visit
the Root and LockGCThing APIs next.

You might study the literature on garbage collection.  A fundamental notion is
that of the "root" or "remembered" set of pointers that root the live graph of
GC-able objects (JS calls these GC-things, to avoid overloading "object").  In
the JS API, a root is a pointer allocated in user-defined storage, registered by
its address, and traced during the mark phase to keep whatever GC-things that
are reachable from it alive.  Note that the rooted pointer (or root, for short)
may point to several different GC-things over its lifetime as a root.

In contrast, if you lock a particular GC-thing, say an object, you are pinning
it into GC'ed memory so that it can't be collected.  There may be many pointers
all of the same value referring to it.  None of them is a root.  They are safe
only so long as it's rooted.

/be

Comment 10

[Phil Schwartau]

Thanks to all; marking Verified - 

Comment 11

[Phil Schwartau]

>  When would I use JS_AddRoot or JS_AddNamedRoot?

Note this comment by Brendan from another discussion: 

"http://lxr.mozilla.org/mozilla/search?string=JS_AddRoot shows too many 
JS_AddRoot calls from js/src/liveconnect -- JS_AddNamedRoot is preferred, 
as a string constant identifying the source callsite can be passed and 
associated with the root via he->value.  Maybe the quickest course is to 
change all these to JS_AddNamedRoot and reproduce?"

Comment 12

[Brendan Eich [:brendan]]

Wrong word at the end of my last comment:

"In contrast, if you lock a particular GC-thing, say an object, you are pinning
it into GC'ed memory so that it can't be collected.  There may be many pointers
all of the same value referring to it.  None of them is a root.  They are safe
only so long as it's rooted."

I meant to write "only so long as it's locked."

/be