1008400 - DataStore does not invoke DETH Traverse/Unlink

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

Data store is declared with:
  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DataStore, DOMEventTargetHelper)
But then the actual declaration of Traverse/Unlink is:
  NS_IMPL_CYCLE_COLLECTION(DataStore, mStore)
...so the CC isn't going to actually trace the wrapper cache.

I'm not sure how bad this really is, because I think the wrapper will still be rooted in the GC, but I'm not entirely sure of what the consequences are, and it seems like this recently landed, so we should just backport it.

Found while working on a patch for bug 1008348.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: DataStore should invoke its parent's Traverse/Unlink.

try run: https://tbpl.mozilla.org/?tree=Try&rev=6cabd1d69357

Comment 2

[Boris Zbarsky [:bzbarsky]]

> because I think the wrapper will still be rooted in the GC

But cycles through it won't be known to the cycle collector, then?

Seems like this must be either a GC hazard or a leak...

Comment 3

[Gene Lian (I already quit Mozilla)]

Thanks Andrew for catching this! \^O^/

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Seems like this must be either a GC hazard or a leak...

Yes, it will certainly cause a leak in certain situations.

Not telling the CC about something holding JS alive can cause the CC to unlink something that is actually alive.  By itself, that will just cause null-derefs, but in concert with another error (where a class leaves dangling pointers after it is unlinked) that can cause sec-crits.

Comment 5

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f680de9c55

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5f680de9c55

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

Andrew, I'm assuming that since this was found via audit that we don't have a test case or steps to reproduce, so I'm marking qe-verify-. Feel free to let me know if that changes or if you'd like us to do more here. Thank you.