OggDemuxer / OggReader are leaking their OggCodecState

RESOLVED INVALID

Status

()

Core
Audio/Video: Playback
RESOLVED INVALID
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

(Assignee)

Description

2 years ago
The OggCodecState are allocated via OggCodecState::Create

but those objects are never deleted.

Found working on 1195723; yet it never showed with ASAN before.

https://treeherder.mozilla.org/logviewer.html#?job_id=25959187&repo=try#L4850
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8782776 [details]
Bug 1296533: [ogg] P1. Fix memory leak in OggDemuxer.

https://reviewboard.mozilla.org/r/72828/#review70556

::: dom/media/ogg/OggDemuxer.h:259
(Diff revision 1)
>  
>    // Map of codec-specific bitstream states.
>    OggCodecStore mCodecStore;
>  
>    // Decode state of the Theora bitstream we're decoding, if we have video.
> -  TheoraState* mTheoraState;
> +  nsAutoPtr<TheoraState> mTheoraState;

Sure you cannot use UniquePtr there and below? (nsAutoPtr is on the way out.)
You can use .get() where you need an non-owning raw pointer.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8782776 [details]
Bug 1296533: [ogg] P1. Fix memory leak in OggDemuxer.

https://reviewboard.mozilla.org/r/72828/#review70558

(Sorry, clicked Publish too quickly before)
r+, whether you address the issue or ignore it.
Attachment #8782776 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8782777 [details]
Bug 1296533: [ogg] P2. Fix memory leak in OggReader.

https://reviewboard.mozilla.org/r/72830/#review70560

::: dom/media/ogg/OggReader.h:258
(Diff revision 1)
>    void SetupMediaTracksInfo(const nsTArray<uint32_t>& aSerials);
>  
>    OggCodecStore mCodecStore;
>  
>    // Decode state of the Theora bitstream we're decoding, if we have video.
> -  TheoraState* mTheoraState;
> +  nsAutoPtr<TheoraState> mTheoraState;

UniquePtr if possible and reasonable.
Attachment #8782777 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8782777 [details]
Bug 1296533: [ogg] P2. Fix memory leak in OggReader.

https://reviewboard.mozilla.org/r/72830/#review70560

> UniquePtr if possible and reasonable.

UniquePtr isn't directly exchangeable with a raw pointer like nsAutoPtr is. raw pointers are passed everywhere in this code as all the CodecStateStore can be changed interchangeable.

Using UniquePtr would require to modify every single line accessing the pointer ; only to pass get() everywhere, losing the entire purpose of UniquePtr that we don't pass pointers around
(Assignee)

Comment 9

2 years ago
The OggCodecState are stored in the OggCodecStore which is using a nsClassHashtable, which put all pointers in a nsAutoPtr already.

So it's not leaking there. misread ASAN report
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Resolution: FIXED → INVALID
(Assignee)

Updated

2 years ago
Attachment #8782776 - Attachment is obsolete: true
(Assignee)

Updated

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