Closed Bug 557479 Opened 10 years ago Closed 9 years ago

leaks 1 instance of VideoData (plus 1 instance of Image sometimes) during mochitest-plain-1

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set

Tracking

()

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

People

(Reporter: masayuki, Assigned: cajbir)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270546101.1270547085.28140.gz

> == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 3783
> 
>      |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
>                                               Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
>    0 TOTAL                                          15       68 339164721        1 (41819.58 +/- 21935.10) 88137076        0 (10129.75 +/- 20756.86)
>  136 VideoData                                      68       68     3372        1 (   52.17 +/-    48.46)        0        0 (    0.00 +/-     0.00)
> 
> nsTraceRefcntImpl::DumpStatistics: 1195 entries
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 68 bytes during test execution
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 68 bytes
> 
> INFO | runtests.py | Running tests: end.
> program finished with exit code 0
> elapsedTime=918.833239
> TinderboxPrint: mochitest-plain-1<br/>56656/0/754&nbsp;<em class="testfail">LEAK</em>
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270844624.1270845998.27021.gz#err0
Linux mozilla-central debug test mochitests-1/5 on 2010/04/09 13:23:44  
s: moz2-linux-slave02
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273531046.1273533519.21887.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/05/10 15:37:26
s: moz2-linux-slave01

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 76 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 76 bytes
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273652563.1273654409.3314.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/05/12 01:22:43

s: win32-slave10
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 80 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 80 bytes
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274462472.1274463598.19125.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/05/21 10:21:12
s: moz2-linux-slave25

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 48 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Image with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 32 bytes
Blocks: 438871
Summary: leaks VideoData during mochitest-plain → leaks 1 instance of VideoData (plus 1 instance of Image sometimes) during mochitest-plain-1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275288346.1275289618.23516.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/05/30 23:45:46
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276023509.1276024637.3408.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/06/08 11:58:29
s: moz2-linux-slave18
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276036142.1276037342.32200.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/06/08 15:29:02
s: moz2-linux-slave18
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276065533.1276069890.18668.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitests-1/5
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276118026.1276119155.24436.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/06/09 14:13:46
s: moz2-linux-slave28
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276231028.1276232101.32477.gz
Linux mozilla-central debug test mochitests-1/5 on 2010/06/10 21:37:08
s: moz2-linux-slave23
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276543692.1276547833.21338.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitests-1/5 on 2010/06/14 12:28:12
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276856448.1276857528.7834.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/06/18 03:20:48
s: mw32-ix-slave24
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277670473.1277672208.25868.gz
Linux tryserver debug test mochitests-1/5 on 2010/06/27 13:27:53

(on a mozilla-central from yesterday or the day before)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281311426.1281313138.15308.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-1/5 on 2010/08/08 16:50:26
s: talos-r3-leopard-024

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 56 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Image with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 40 bytes
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281365462.1281367097.20881.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-1/5 on 2010/08/09 07:51:02
s: talos-r3-leopard-009

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Image with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 40 bytes
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281599397.1281601044.2247.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-1/5 on 2010/08/12 00:49:57
s: talos-r3-leopard-011

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 56 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Image with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of VideoData with size 40 bytes
OS: Linux → All
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283810246.1283811964.32469.gz
OS X 10.5 comm-central-trunk debug test mochitests-1/5 
s: cb-sea-miniosx02
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1284002767.1284004466.22152.gz
OS X 10.5 comm-central-trunk debug test mochitests-1/5 
s: cb-sea-miniosx02
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1284009720.1284011433.20515.gz
OS X 10.5 comm-central-trunk debug test mochitests-1/5 
s: cb-sea-miniosx02
Can we get this to show up in tbpl somehow?
(In reply to comment #92)
> Can we get this to show up in tbpl somehow?

File a bug?
nsOggReader::DecodeVideoFrame has a 'frames' array that contains heap allocated VideoData instances. It also has multiple early returns - could one of these returns be happening (or an assertion hitting?) that results in video data in that array not being deleted?
Attached patch Early exit handling (obsolete) — Splinter Review
This handles one of the early exits by breaking out and still processing any frames that have been decoded. What are your thoughts on this Chris? Or should it just Clear the frames?
Attachment #474910 - Flags: review?(chris)
We should convert this stuff over to nsAutoPtrs.
Comment on attachment 474910 [details] [diff] [review]
Early exit handling

I think we need to ensure the frames array is cleared there, if we set endOfStream and break we'll end up pushing video frames which don't have timestamps onto the queue.

Matthew's suggestion of converting frames to an autoptrs is a good idea.
For autoptr usage, something like this?
Attachment #474910 - Attachment is obsolete: true
Attachment #474945 - Flags: review?(chris)
Attachment #474945 - Flags: feedback?(kinetik)
Attachment #474910 - Flags: review?(chris)
Yeah.  From a very quick look, DecodeAudioData might need similar treatment for the SoundData "chunks" temporary array.

One nit:

+    if (!aKeyframeSkip || (aKeyframeSkip && data->mKeyframe)) {
+      if (aKeyframeSkip && data->mKeyframe) {
         aKeyframeSkip = PR_FALSE;
       }
-    } else {
-      frames[i] = nsnull;
-      data = nsnull;
+      mVideoQueue.Push(data.forget());
     }
   }

This logic could be simpler, I think.
Comment on attachment 474945 [details] [diff] [review]
Use autoptr to fix possible memory leaks

The Vorbis decode code is indeed vulnerable to this too, we should fix it while we're here.

>diff --git a/content/media/ogg/nsOggReader.cpp b/content/media/ogg/nsOggReader.cpp
>+AllFrameTimesIncrease(nsTArray<nsAutoPtr<VideoData> >& aFrames)
> {
>   PRInt64 prevTime = -1;
>   PRInt64 prevGranulepos = -1;
>   for (PRUint32 i = 0; i < aFrames.Length(); i++) {
>-    VideoData* f = aFrames[i];
>+    VideoData* f = aFrames[i].get();

With MSVC at least you don't need the .get(), as nsAutoPtr has an operator*.

>diff --git a/content/media/ogg/nsOggReader.h b/content/media/ogg/nsOggReader.h
>index 1ec3c0b..279d68f 100644
>--- a/content/media/ogg/nsOggReader.h
>+++ b/content/media/ogg/nsOggReader.h
>@@ -151,17 +151,17 @@ private:
>-  nsresult DecodeTheora(nsTArray<VideoData*>& aFrames,
>+  nsresult DecodeTheora(nsTArray<nsAutoPtr<VideoData> >& aFrames,
>                         ogg_packet* aPacket);

Nit: Remove ' ' in "> >&" ?
(In reply to comment #100)
> >+  nsresult DecodeTheora(nsTArray<nsAutoPtr<VideoData> >& aFrames,
> >                         ogg_packet* aPacket);
> 
> Nit: Remove ' ' in "> >&" ?

It's necessary until C++0x is widespread, unfortunately: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1757.html
This addresses Matthew's and Chris' review comments.
Assignee: nobody → chris.double
Attachment #474945 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #475260 - Flags: review?(chris)
Attachment #474945 - Flags: review?(chris)
Attachment #474945 - Flags: feedback?(kinetik)
Attachment #475260 - Flags: review?(chris) → review+
Comment on attachment 475260 [details] [diff] [review]
Use autoptr to fix possible memory leaks

This should solve an intermittent orange and it would be good to land - can we get approval?
Attachment #475260 - Flags: approval2.0?
Attachment #475260 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/ffc8a67bfc6b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.