Closed
Bug 934698
Opened 11 years ago
Closed 11 years ago
Analysis false positive with AddRef()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: sfink, Assigned: sfink)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
One hacky option would be to whitelist any .AddRef FieldCall for a subclass overriding AddRef if it triggers any hazards.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Attachment #827117 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
Pretty much the same patch, except this version actually works. :-)
Attachment #827224 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•11 years ago
|
Attachment #827117 -
Attachment is obsolete: true
Attachment #827117 -
Flags: review?(bhackett1024)
Updated•11 years ago
|
Attachment #827224 -
Flags: review?(bhackett1024) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•