Subclasses of HTMLMediaElement that implement their own AddRef/Release methods cause negative leak reports
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
Details
Attachments
(1 file, 3 obsolete files)
Special thanks to mccr8 and smaug who patiently walked me through this today. (Please let me know if any of the below is inaccurate).
I've been dealing with an issue in 1521964 where I've been getting negative leaks reported when pushing to automation.
I got pretty lucky and captured the problem in rr, and what I found is a pretty grim tail.
Basically, we have some stuff inside of the HTMLMediaElement constructor that calls HTMLMediaElement::AddRef.
One of my patches, in order to avoid cycles, has HTMLVideoElement (a subclass of HTMLMediaElement) override AddRef, Release and QueryInterface with its own implementations (via the NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0 macro).
The problem is that we can get into a situation like this - where the HTMLVideoElement is constructed, and it delegates to its its parent class (HTMLMediaElement) constructor, which AddRefs.
However, because we're inside of a constructor, and the vtable for the HTMLVideoElement hasn't been set up yet, it calls HTMLMediaElement::AddRef, and not HTMLVideoElement::AddRef!
This means that we record a refcount increase for an HTMLMediaElement, and when the <video> element finally goes away, we decrement for a HTMLVideoElement, which causes us to have negative leaks (unless we're logging - in which case, we'll crash here instead, since there's no serial recorded for the subclass).
|
Assignee | |
Comment 1•6 years ago
|
||
smaug suggests we move what's going on in the HTMLMediaElement constructor into a separate ::Init() method, and then expand out the NS_IMPL_NS_NEW_HTML_ELEMENT for HTMLVideoElement and HTMLAudioElement, and have them call that Init() method after construction completes.
That way, AddRef / Release's should be called on the right class.
There's a pre-existing ::Init on HTMLVideoElement, but I think I can get away with naming it something better.
|
||
Comment 2•6 years ago
|
||
Would it be possible to get static analysis to detect this pattern?
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 3•6 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
Would it be possible to get static analysis to detect this pattern?
That seems like it'd be a good idea, but I'm not sure how to do that. Who manages our static analysis stuff these days?
|
||
Comment 4•6 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
Would it be possible to get static analysis to detect this pattern?
MSVC does have a warning that is off by default about using |this| in the initializer list:
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-c4355
I'm not sure what Clang has.
The problem is that sometimes it is okay and sometimes it is not, so you end up having to annotate things to disable the warning. I think a static analysis would have to be global in order to be accurate.
For instance, in HTMLMediaElement this is not okay because it ends up creating a strong reference:
mAudioTrackList(new AudioTrackList(OwnerDoc()->GetParentObject(), this)),
but this is okay because this does not:
mAudioChannelWrapper(new AudioChannelAgentCallback(this)),
Smaug had an idea that we could do a dynamic analysis where we check that an object has a refcount of 0 and is not purple (which indicates that it has not been released) after the ctor, but you'd have to somehow insert these checks in enough places for it to be useful.
|
|
Assignee | |
Comment 5•6 years ago
|
||
|
||
Comment 7•6 years ago
|
||
Backed out changeset 28bc841f06fc (bug 1530862) for failing HTMLMediaElement
push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=230946706&revision=28bc841f06fc5f4d01defbacc80e017f701a8d57
backout: https://hg.mozilla.org/integration/autoland/rev/de132128473b4973ba81fb84a75ee87f2ae55b7b
|
|
||
Comment 8•6 years ago
|
||
Ah, yeah, I guess you can't really use CLONE_WITH_INIT and keep around this assertion, because that takes a strong reference before calling Init.
|
||
Comment 10•6 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 11•6 years ago
|
||
Depends on D22160
|
|
Assignee | |
Comment 12•6 years ago
|
||
Depends on D22163
|
|
Assignee | |
Comment 13•6 years ago
|
||
Depends on D22164
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
|
||
Comment 14•6 years ago
|
||
Comment on attachment 9048633 [details]
Bug 1531101 - When the PictureInPicture window closes, stop cloning immediately to update the original video UAWidget. r?felipe
Revision D22165 was moved to bug 1531101. Setting attachment 9048633 [details] to obsolete.
|
|
||
Comment 15•6 years ago
|
||
Comment on attachment 9048632 [details]
Bug 1531101 - Add strings, styles and mark-up for Picture-in-Picture message for a video with controls. r?jaws
Revision D22164 was moved to bug 1531101. Setting attachment 9048632 [details] to obsolete.
|
|
||
Comment 16•6 years ago
|
||
Comment on attachment 9048629 [details]
Bug 1531101 - Fire UAWidget setup / change events when starting or stopping a visual clone of a <video> element. r?jya
Revision D22161 was moved to bug 1531101. Setting attachment 9048629 [details] to obsolete.
Description
•