Closed Bug 1297343 Opened 5 years ago Closed 5 years ago

Make some more object classes background finalized


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: jonco, Assigned: jonco)



(2 files, 1 obsolete file)

Attached patch make-more-things-bg-finalized (obsolete) — Splinter Review
Following the previous finalization changes, here's a patch to make some more things background finalized.

For foreground finalized things I also added and assertion to the finalizer that it's called on the main thread.  Sometimes background finalized things can be finalized on the main thread too so we can't add the opposite assert for them.
Attachment #8783901 - Flags: review?(sphink)
Comment on attachment 8783901 [details] [diff] [review]

Review of attachment 8783901 [details] [diff] [review]:

We could add a state variable saying that we're doing bg finalization, just to assert on it regardless of which thread we're actually running on. You'd have a better idea than I would as to whether it'd be worth it; I'm fine landing this as-is.

::: js/src/vm/SharedArrayObject.cpp
@@ +300,5 @@
>  void
>  SharedArrayBufferObject::Finalize(FreeOp* fop, JSObject* obj)
>  {
> +    // Note: This can be executed off main thread.

This is the only scary one, but I think you're right, it looks safe.
Attachment #8783901 - Flags: review?(sphink) → review+
Attached patch finalize-updatesSplinter Review
Oh, we can just ensure that the runtime pointer is always null when we do background finalization regardless of thread.  Then we can add a maybeOffMainThread() method that we can check with assertions.
Attachment #8784352 - Flags: review?(sphink)
I replaced the comments for finalizers called in the background with assertions on fop->maybeOffMainThread().  Carrying r+.
Attachment #8783901 - Attachment is obsolete: true
Attachment #8784353 - Flags: review+
Comment on attachment 8784352 [details] [diff] [review]

Review of attachment 8784352 [details] [diff] [review]:

I like it.
Attachment #8784352 - Flags: review?(sphink) → review+
Pushed by
Make some more object classes background finalized r=sfink
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.