765432 - Make JS_IsAboutToBeFinalized indirect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Terrence Cole [:terrence]]

The tables in XPCMaps.h (see JSObject2JSObjectMap for an example) need to be able to relocate objects when sweeping, since they are weak.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0: exact typing

Types, do you speak them?

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0: indirect - JS part

This makes JS_IsAboutToBeFinalized take a JSObject** and adds a method to ObjectPtr so we don't have to do any awkward get() + update + set() dance on the underlying object.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0: indirect - XPConnect part

This adapts XPConnect to use the new, indirect JS_IsAboutToBeFinalized.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 729342 [details] [diff] [review]
v0: indirect - XPConnect part

Can you explain, preferably in a comment above the JS_IsAboutToBeFinalized declaration in jsapi.h, why the argument needs to be indirect? Does it get relocated? In what circumstances? Without context, introducing a side-effect into a predicate function like this seems kind of scary. I'm open to being convinced that this happens to be a very convenient and clever place to do this work, but I'd like that to be explained in excruciating detail.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 729341 [details] [diff] [review]
v0: indirect - JS part

Review of attachment 729341 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the comment Bobby requested. Also, I don't think you meant to remove testIsAboutToBeFinalized.cpp entirely. If you did, please explain.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #5)
> Please add the comment Bobby requested. Also, I don't think you meant to
> remove testIsAboutToBeFinalized.cpp entirely. If you did, please explain.

testIsAboutToBeFinalized.cpp is not built or run with jsapi-tests and indeed doesn't compile anymore because of a missing jsvalRoot. When it did run, it was meant to test that JS_IsAboutToBeFinalized special cased for "unit strings" after bug 528645. Since I've never heard of "unit strings" and we don't specialize IsAboutToBeFinalized anymore, I thought it would be best to just remove the test.

I suppose we could instead modernize and re-purpose it to act as a baseline functionality test for the GC, but I'm not sure how much value this would have given how many other leak tests we have.

Comment 7

[Terrence Cole [:terrence]]

Here is the comment I came up with. Proofing would be greatly appreciated.

/*
 * Checks if the referenced object is going to be finalized at the end of the
 * current GC. When called outside of the context of a GC, this method will
 * return false.
 *
 * This method is indirect - it takes a pointer to a JSObject* instead of a
 * JSObject* directly - to support weak references in the context of a moving
 * GC. At the end of a GC, all weak references must either be marked by some
 * other edge in the object graph, or nulled out. Since weak references are
 * never marked directly, the GC instead intercepts the check weak references
 * must make to see if the reference must be removed.
 *
 * When this method returns false and a GC is in progress, the referenced
 * object may have been moved. Callers of this method are responsible for
 * updating any state that is dependent on the value of *obj; for example,
 * hash table constraints or flags embedded in the low bits.
 */

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, you're right, the test looks pointless.

Here's a revised version of the comment.

"JS_IsAboutToBeFinalized checks if the given object is going to be finalized at the end of the current GC. When called outside of the context of a GC, this function will return false. Typically this function is used on weak references, where the reference should be nulled out or destroyed if the given object is about to be finalized.

The argument to JS_IsAboutToBeFinalized is an in-out param: when the function returns false, the object being referenced is still alive, but the garbage collector might have moved it. In this case, the reference passed to JS_IsAboutToBeFinalized will be updated to the object's new location. Callers of this method are responsible for updating any state that is dependent on the object's address. For example, if the object's address is used as a key in a hashtable, then the object must be removed and re-inserted with the correct hash."

Bobby, does this look okay? We could add more about specifically when stuff gets moved, but that's an implementation detail that I'd rather not put in the documentation. Right now, any object without a finalizer can be moved. However, we may lift that restriction in the future.

It might be smart to have a more general comment in GCAPI.h that discusses marking. There are really two kinds of pointers--strong and weak. Strong pointers are marked via JS_CallTracer, which will soon take an in-out param. Weak pointers are handled via JS_IsAboutToBeFinalized, which this bug fixes. Either way, we have a way of updating the reference if the referent moves.

Comment 9

[Bobby Holley (:bholley)]

This sounds good. But note that this basically only applies to weak references in XPConnect, which hooks into GC callbacks and can actually use this function usefully. The DOM also implements weak references with finalize hooks that null out a JSObject*, which will still need to be handled. That is to say, this is not the only mechanism by which weak references are implemented.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) from comment #9)
> This sounds good. But note that this basically only applies to weak
> references in XPConnect, which hooks into GC callbacks and can actually use
> this function usefully. The DOM also implements weak references with
> finalize hooks that null out a JSObject*, which will still need to be
> handled. That is to say, this is not the only mechanism by which weak
> references are implemented.

As we don't currently move stuff with finalizers, that is not yet a problem. But I see your point. When we do compacting GC (or when we allow objects with finalizers to be allocated in the nursery), we will probably add a move hook to JSClass that could update the wrapper cache entry and do whatever else needs to be done for these sorts of pointers.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 729342 [details] [diff] [review]
v0: indirect - XPConnect part

Review of attachment 729342 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with the new comment in place.

Comment 12

[Terrence Cole [:terrence]]

Green try at:
https://tbpl.mozilla.org/?tree=Try&rev=d074f8d6e87a

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88ec6ee2d57a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88ec6ee2d57a