Closed Bug 1514409 Opened 10 months ago Closed 9 months ago

Rooting hazard in ModuleNamespaceObject::create

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file)

Ok, I know one reason this is a false positive, but I'm not sure what to do here.

ModuleNamespaceObject::create(..., UniquePtr<IndirectBindingMap> bindings, ...) has a hazard because 'bindings' is a UniquePtr and is therefore regarded as something that is not going to be traced during a GC. And it's a map from IndirectBindingMap::Binding to something, and Bindings contain HeapPtr<Shape*>. So in theory you could have a bunch of shape keys in the map and GC. Nothing is obviously going to trace those shapes.

This is safe, because currently there's a single caller than passes in an empty bindings map.

The most obvious fix would be to remove the parameter and create an empty bindings locally, but that's different.

Another fix would be to pass in a Handle<IndirectBindingMap>, but I don't think that makes sense here -- bindings is moved into the ModuleNamespaceObject, so we want to transfer ownership (hence the UniquePtr).

Oh, wait, I know! If we handed over ownership to a local Rooted<IndirectBindingMap>, all would be good. Hopefully there's a way to make that work...?
This compiles. No clue whether it's doing anything sane. I'm not sure when this code runs. I'll do a try push.
Attachment #9031659 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 9031659 [details] [diff] [review]
Transfer ownership of bindings

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

LGTM.
Attachment #9031659 - Flags: review?(jcoppeard) → review+
Priority: -- → P3

Can this land or is this blocked on hazard analysis improvements?

Flags: needinfo?(sphink)
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7d7a7e6d6d
Transfer ownership of bindings, r=jonco
Flags: needinfo?(sphink)
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.