Closed Bug 603912 Opened 9 years ago Closed 9 years ago

nsWebMReader::DecodeVideoFrame leaks packet when s->GetEndMediaTime() returns -1

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: timeless, Assigned: kinetik)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, memory-leak)

Attachments

(1 file, 1 obsolete file)

550 PRBool nsWebMReader::DecodeVideoFrame(PRBool &aKeyframeSkip,
551                                       PRInt64 aTimeThreshold)
552 {

557   nestegg_packet* packet = NextPacket(VIDEO);

585   // The end time of this frame is the start time of the next frame.  Fetch
586   // the timestamp of the next packet for this track.  If we've reached the
587   // end of the stream, use the file's duration as the end time of this
588   // video frame.
589   uint64_t next_tstamp = 0;
590   {
591     nestegg_packet* next_packet = NextPacket(VIDEO);
592     if (next_packet) {
593       r = nestegg_packet_tstamp(next_packet, &next_tstamp);
594       if (r == -1) {
595         nestegg_free_packet(next_packet);
596         return PR_FALSE;

599     } else {
600       MonitorAutoExit exitMon(mMonitor);
601       MonitorAutoEnter decoderMon(mDecoder->GetMonitor());
602       nsBuiltinDecoderStateMachine* s =
603         static_cast<nsBuiltinDecoderStateMachine*>(mDecoder->GetStateMachine());
604       PRInt64 endTime = s->GetEndMediaTime();
605       if (endTime == -1) {
606         return PR_FALSE;
Nice find.  We should probably wrap nestegg_packets in an RAII auto-deleter.
blocking2.0: --- → ?
Yeah that would be a good idea. At least it isn't actually leaked permanently - it gets released when the nestegg context is destroyed, right?
If you're thinking of nestegg's pool allocator, that's only used for internal memory management and isn't exposed via the user API.  So you must call nestegg_free_packet for any packet you get from nestegg_read_packet.
fwiw, if i file a coverity bug about a memory leak, it's because coverity believes that it's permanently leaked.
Attached patch simple patch (obsolete) — Splinter Review
this is the obvious patch. if you want to do something more complicated, that's up to you :)
Attachment #482973 - Flags: review?(chris.double)
Looks okay, but we also need to free |packet| in this path:

594       if (r == -1) {
595         nestegg_free_packet(next_packet);
596         return PR_FALSE;
Comment on attachment 482973 [details] [diff] [review]
simple patch

I'll pass the review to Matthew since he's the nestegg expert.
Attachment #482973 - Flags: review?(chris.double) → review?(kinetik)
Convert most handling of nestegg_packet pointers over to nsAutoRef.  Packets in the PacketQueue are still managed manually by the queue.

Requesting feedback from karlt on top of regular review to make sure I'm using nsAutoRef as nicely as I can.  There are two slightly ugly things:

- returning "NULL" from NextPacket requires slightly gross syntax.

- couldn't find a nicer way to get the raw nestegg_packet pointer from nestegg_read_packet in NextPacket.
Assignee: nobody → kinetik
Attachment #482973 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #483050 - Flags: review?(chris.double)
Attachment #483050 - Flags: feedback?(karlt)
Attachment #482973 - Flags: review?(kinetik)
Attachment #483050 - Flags: review?(chris.double) → review+
Comment on attachment 483050 [details] [diff] [review]
complicated patch

(In reply to comment #8)
> Requesting feedback from karlt on top of regular review to make sure I'm using
> nsAutoRef as nicely as I can.  There are two slightly ugly things:

I think this is as nicely as you can.

> - returning "NULL" from NextPacket requires slightly gross syntax.

I thought about a non-explicit nsReturnRef(int) constructor that asserted that
the parameter is 0, but T is not necessary a pointer and may in fact be int.

I guess we could have a untemplated nsVoidRef and nsReturnRef(nsVoidRef) so
that return nsVoidRef() would work.
Don't know whether the terseness is worth the namespace pollution, and I actually like being reminded of the type that I'm returning.

Another option is packet.reset(); return packet.out(), but that's kind of less
clear.

> - couldn't find a nicer way to get the raw nestegg_packet pointer from
> nestegg_read_packet in NextPacket.

I don't really have any good suggestings.
I didn't do a nsGetterAddRefs for raw outparameters.
Attachment #483050 - Flags: feedback?(karlt) → feedback+
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/ee16bb4a5c95
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Backed out due to errors in caused by one of the changesets I pushed this with.

http://hg.mozilla.org/mozilla-central/rev/fefd669994a6
http://hg.mozilla.org/mozilla-central/rev/168dd8a1736d

Some of the errors:

Rev3 Fedora 12 mozilla-central opt test mochitests-5/5
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287262011.1287262859.26658.gz

19554 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - [object Event] at undefined:undefined
Thread 0 (crashed)
0  libxul.so!nsMediaCacheStream::Close [nsMediaCache.cpp:f9f10c04dceb : 1891 + 0x6]
1  libxul.so!nsMediaChannelStream::Close [nsMediaStream.cpp:f9f10c04dceb : 498 + 0x8]
2  libxul.so!nsWaveDecoder::Stop [nsWaveDecoder.cpp:f9f10c04dceb : 1372 + 0x8]

19557 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols_audio_direction.html | Test timed out.
19560 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols_video_direction.html | Test timed out.

Rev3 Fedora 12 mozilla-central debug test reftest
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287261600.1287263280.28360.gz
Lots of failures...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The failures were caused by bug 572579. Landed again without that...

http://hg.mozilla.org/mozilla-central/rev/bfe85b4ed5cd
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.