Closed Bug 765432 Opened 9 years ago Closed 8 years ago

Make JS_IsAboutToBeFinalized indirect

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(3 files)

The tables in XPCMaps.h (see JSObject2JSObjectMap for an example) need to be able to relocate objects when sweeping, since they are weak.
Attached patch v0: exact typingSplinter Review
Types, do you speak them?
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #729340 - Flags: review?(wmccloskey)
Attachment #729340 - Attachment description: v0 → v0: exact typing
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.
Attachment #729341 - Flags: review?(wmccloskey)
This adapts XPConnect to use the new, indirect JS_IsAboutToBeFinalized.
Attachment #729342 - Flags: review?(bobbyholley+bmo)
Blocks: 764882
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.
Attachment #729342 - Flags: review?(bobbyholley+bmo)
Attachment #729340 - Flags: review?(wmccloskey) → review+
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.
Attachment #729341 - Flags: review?(wmccloskey) → review+
(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.
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.
 */
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.
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.
(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 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.
Attachment #729342 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/88ec6ee2d57a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.