1049167 - HoldJSObjects/DropJSObjects audit

Status: NEW

(Core :: Cycle Collector, defect)

Description

[Andrew McCreight [:mccr8]]

HoldJSObjects should only be called in the constructor, DropJSObjects should only be called in the destructor.  Failure to do that can interact with other weirdness to lead to use-after-free problems.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I think calling HoldJSObjects only in ctor is too strict. It makes us to put more than needed objects to jsholders.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, I suppose that's true.  I could do some profiling of what % of each object class ends up holding JS alive, though I guess it will be hard to come up with test cases for that.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Most of the DOM Nodes don't preserve wrapper, so they don't hold js objects by default.

Comment 4

[Andrew McCreight [:mccr8]]

I wasn't planning on messing with nodes, just the random misc. junk we have floating around.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ah, in that case calling HoldJSObjects always might work.

Comment 6

[Thomas Wisniewski]

Was there anything left to do here Andrew, or was this meta-bug just waiting on its dependencies to be fixed?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Thomas Wisniewski from comment #6)
> Was there anything left to do here Andrew, or was this meta-bug just waiting
> on its dependencies to be fixed?

There's probably more to do. I got distracted in the middle of my audit so there's probably more to fix.

Comment 8

[Andrew McCreight [:mccr8]]

One way to address would be to have some kind of HoldDropJSObjects<T> class that calls Hold/Drop appropriately. It would work because the type T is available. Then you'd add a static assert somewhere in the TRACE macros to check that T derives from HoldDropJSObjects<T>. You'd need some special handling for wrapper cached objects, because they lazily do hold/drop depending on whether the wrapper is preserved. You'd also want to handle the case where a wrapper cached class adds its own stuff to trace, which necessitates eager Holding. You can probably do that by just making the empty wrapper cache tracing macro thing not do the static assert, but as soon as you add anything else, you have the assert again.