1514409 - Rooting hazard in ModuleNamespaceObject::create

Status: RESOLVED FIXED

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

Description

[Steve Fink [:sfink] [:s:]]

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...?

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Transfer ownership of bindings

This compiles. No clue whether it's doing anything sane. I'm not sure when this code runs. I'll do a try push.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 9031659 [details] [diff] [review]
Transfer ownership of bindings

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

LGTM.

Comment 3

[Jon Coppeard (:jonco)]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7d7a7e6d6d
Transfer ownership of bindings, r=jonco

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4f7d7a7e6d6d