Closed Bug 1295991 Opened 3 years ago Closed 3 years ago

Make runtime pointer null in FreeOps used off main thread

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

Currently the FreeOp always carries a pointer to the JSRuntime.  We should make this null if the FreeOp can be used off main thread so that finalizers can't do non-threadsafe things with it.
I had to delay initialisation of the runtime's default free op until after ownerThread_ was initialised so that CurrentThreadCanAccessRuntime worked.
Attachment #8782016 - Flags: review?(sphink)
Comment on attachment 8782016 [details] [diff] [review]
bug1295991-free-op-runtime

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

::: js/src/vm/TypedArrayObject.cpp
@@ +159,1 @@
>          js_free(curObj->elements());

Sorry, can you explain this change? I'm confused by all the permutations of where data can be. Why is the nursery.isInside check not needed?

It seems like this is for the case where you nursery-allocated data for a buffer-less typed array. Do those just not exist? I couldn't find what nursery-allocates typed array data in the first place.
Flags: needinfo?(jcoppeard)
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
Sorry, I meant to add an explanation but forget.

Here we're working out whether we need to free the elements of a buffer-less typed array.  We already know the object is tenured, and we don't support tenured objects with nursery allocated elements (we malloc them when we tenure the object).  So the check is not required.
Flags: needinfo?(jcoppeard)
Flags: needinfo?(sphink)
Comment on attachment 8782016 [details] [diff] [review]
bug1295991-free-op-runtime

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

Oh, right, we know we're tenured here. That makes sense, thanks.
Attachment #8782016 - Flags: review?(sphink) → review+
Flags: needinfo?(sphink)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1bbf695558
Don't give FreeOp a runtime pointer if it's used off the main thread r=sfink
https://hg.mozilla.org/mozilla-central/rev/cd1bbf695558
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.