Subclasses of HTMLMediaElement that implement their own AddRef/Release methods cause negative leak reports

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
Rank:
15
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

4 months 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.

Would it be possible to get static analysis to detect this pattern?

Updated

4 months ago
Rank: 15
Priority: -- → P2
Assignee

Comment 3

4 months 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?

Assignee: nobody → mconley

(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.

Comment 6

4 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28bc841f06fc
Add an Init() method to HTMLMediaElement to be called right after construction to do any AddRef / Release-ing. r=jya,mccr8,smaug

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 9

4 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c13d34779fe8
Add an Init() method to HTMLMediaElement to be called right after construction to do any AddRef / Release-ing. r=jya,mccr8,smaug

Comment 10

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Updated

4 months ago
Flags: needinfo?(mconley)
Attachment #9048629 - Attachment description: Bug 1530862 - Fire UAWidget setup / change events when starting or stopping a visual clone of a <video> element. r?jya → Bug 1531101 - Fire UAWidget setup / change events when starting or stopping a visual clone of a <video> element. r?jya
Attachment #9048632 - Attachment description: Bug 1530862 - Add strings, styles and mark-up for Picture-in-Picture message for a video with controls. r?jaws → Bug 1531101 - Add strings, styles and mark-up for Picture-in-Picture message for a video with controls. r?jaws
Attachment #9048633 - Attachment description: Bug 1530862 - When the PictureInPicture window closes, stop cloning immediately to update the original video UAWidget. r?felipe → Bug 1531101 - When the PictureInPicture window closes, stop cloning immediately to update the original video UAWidget. r?felipe

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.

Attachment #9048633 - Attachment is obsolete: true

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.

Attachment #9048632 - Attachment is obsolete: true

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.

Attachment #9048629 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.