Closed Bug 1001355 Opened 6 years ago Closed 6 years ago

IsAboutToBeFinalized overload for VMFunction pointers is backwards

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

I just spotted that IsAboutToBeFinalized(const js::jit::VMFunction **vmfunc) always returns true, when in fact it should always return false.

This was because I renamed the function from IsMarked() in bug 790338 but didn't invert the result.  The same thing happened for the IonCode overload but was fixed in bug 816493.

This means that the functionWrappers_ map in JitRuntime will always be cleared on GC.
Attachment #8412683 - Flags: review?(wmccloskey)
This code seems pretty over-generalized. We appear to have only one use of WeakCache, and in that case the key is a VMFunction*. Can we just assume, in WeakCache, that the key will never be finalized? Then we can remove the weird overload of IsAboutToFinalized for VMFunction. It seems pretty error-prone to have that around since it doesn't do what you expect at all.
(In reply to Bill McCloskey (:billm) from comment #2)
Well the reason I saw this was because Till was talking about adding a new use of WeakCache (from JSScript to LazyScript, or the other way around I don't remember), which would require this to work with keys that can be finalized.

Another option might be to add IsAboutToBeFinalized overload on void* to catch all non-GC things.
Comment on attachment 8412683 [details] [diff] [review]
bug1001355-is-about-to-be-finalized

Well, all right.
Attachment #8412683 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/4b91913320ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.