1180105 - memory leak with mjpeg based webcams

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

Original reported Post http://forums.mozillazine.org/viewtopic.php?p=14220785#p14220785

Nightly process keeps growing on memory until i close the tab (with no add-on enabled).

Steps to reproduce:
1. open http://abbatucci.aliopolis.eu:8090/home.html?src=5&mode=1

Actual results:
Nightly process keeps growing on memory (approx. 5Mbyte/sec).
Reproduced with/without e10s.


Regression window:
GOOD : 20150630115143 https://hg.mozilla.org/integration/mozilla-inbound/rev/c97c48655c82
BAD : 20150630115343 https://hg.mozilla.org/integration/mozilla-inbound/rev/d418eb96a90d
Pushlog : https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d418eb96a90d

Regressed by: Bug 1161627

Comment 1

[Alice0775 White]

Attachment: memory-report.json.gz

Comment 2

[Nathan Froyd [:froydnj]]

Ugh.  Do you need a webcam to reproduce this?

This ought to be easily diagnosed with a debug build, as we're leaking a refcounted thing.  And it's probably something that's not tested in mochitests or similar, or we'd notice (plus the DEBUG-mode asserts for already_AddRefed clearing out its pointer prior to destruction.

I'm out today, so I won't be able to look at this until Monday, and I won't be able to look at this at all without a webcam, if the testcase requires it.  Ehsan, are you able to poke at this with a debug build?

Comment 3

[Alice0775 White]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> Ugh.  Do you need a webcam to reproduce this?
> 

Not need. Just open the url

Comment 4

[(no longer active)]

This is an issue with mjpeg streams serving images faster than we can decode.  test_bug733553.html sort of tests this code path but it waits for each image to fully load before starting to load the next one, which is not quite what we want.  I'll try to write a better test.

Comment 5

[(no longer active)]

Attachment: Do not leak the SourceSurface returned from imgIContainer::GetFrame in BlockUntilDecodedAndFinishObserving

Comment 6

[(no longer active)]

Note that this leak would be caught by the test that I have added as a fatal assertion in ~already_AddRefed.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8629393 [details] [diff] [review]
Do not leak the SourceSurface returned from imgIContainer::GetFrame in BlockUntilDecodedAndFinishObserving

Review of attachment 8629393 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks Ehsan.

::: image/MultipartImage.cpp
@@ +39,5 @@
>    {
>      // Use GetFrame() to block until our image finishes decoding.
> +    nsRefPtr<SourceSurface> surface =
> +      mImage->GetFrame(imgIContainer::FRAME_CURRENT,
> +                       imgIContainer::FLAG_SYNC_DECODE);

The original code leaks?! I thought already_AddRefed<> would free the thing it's pointing to on destruction. Good to know that's not the case.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Seth Fowler [:seth] from comment #7)
> ::: image/MultipartImage.cpp
> @@ +39,5 @@
> >    {
> >      // Use GetFrame() to block until our image finishes decoding.
> > +    nsRefPtr<SourceSurface> surface =
> > +      mImage->GetFrame(imgIContainer::FRAME_CURRENT,
> > +                       imgIContainer::FLAG_SYNC_DECODE);
> 
> The original code leaks?! I thought already_AddRefed<> would free the thing
> it's pointing to on destruction. Good to know that's not the case.

The original code prior to s/TemporaryRef/already_AddRefed/ didn't leak, because TemporaryRef more-or-less functioned as a smart pointer, i.e. Release'ing the pointer upon destruction.  already_AddRefed is stricter: it expects you to have done something with the pointer contained within it and does not Release the pointer itself.

Comment 9

[(no longer active)]

Right.  Also, already_AddRefed() does a fatal assertion in its dtor to make sure its mRawPtr has been nulled out, but we were doubly unfortunate here in that the branch that we called BlockUntilDecodedAndFinishObserving() in was never hit in any of our tests since in the only one test that I could find which hit this code, we would wait for each image frame to load, therefore we would never be blocked on the decoder.  :-)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da0f70cf87a3

Comment 11

[(no longer active)]

BTW I filed bug 1180993 to help us catch future mistakes like this at compile time.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da0f70cf87a3