934698 - Analysis false positive with AddRef()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Hazard:

Function 'JSObject* mozilla::dom::AudioDestinationNodeBinding::Wrap(JSContext*, class JS::Handle<JSObject*>, mozilla::dom::AudioDestinationNode*, nsWrapperCache*)' has unrooted 'obj:1' of type 'JSObject*' live across GC call mozilla::dom::AudioDestinationNode.AddRef at dom/bindings/AudioDestinationNodeBinding.cpp:260
    dom/bindings/AudioDestinationNodeBinding.cpp:260: Call(54,55, aObject*.AddRef*())
    dom/bindings/AudioDestinationNodeBinding.cpp:263: Call(55,56, aCache*.SetWrapper(obj:1*))

The question is why it thinks that mozilla::dom::AudioDestinationNode.AddRef is a GC function.

Comment 1

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

The analysis whitelists nsISupports::AddRef, and any virtual overrides of it, during the callgraph computation. Also, nsISupports.AddRef is specifically whitelisted as a field call.

My current guess is that this is a *direct* call of that function, and so the callgraph special-casing never gets consulted. (We *know* this can call AddRef because we see it in the CFG.) But it's overridden, so the nsISupports.AddRef FieldCall whitelist doesn't apply either.

This is currently triggering 25 false positive hazards in the browser analysis, which are not getting reported in the counts due to a bug in the hazard matching regex.

Comment 2

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

One hacky option would be to whitelist any .AddRef FieldCall for a subclass overriding AddRef if it triggers any hazards.

Comment 3

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

Attachment: Suppress nsISupports.{AddRef,Release} when called directly

Here's a hopefully less-hacky approach, which adds descendants of nsISupports.{AddRef,Release} to the suppressed functions list (which previously only contained direct calls, not field calls.)

It's a little funky, since these calls get added in a callsite-specific way, but used generally.

Comment 4

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

Attachment: Suppress nsISupports.{AddRef,Release} when called directly

Pretty much the same patch, except this version actually works. :-)

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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