503989 - Pending <audio src> loads leak on shutdown (entraining nsDocument)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

This prevents me from looking for other nsDocument leak bugs :(

Comment 4

[Joseph Huckaby]

This bug states that "pending" audio src loads leak on shutdown.  I am seeing that fully completed loads also leak.  For example, if you let all the Audio objects fully load, then refresh the page, no memory is released, and the application's RAM usage continues to grow for every refresh.

Another test case, hosted on an external server:
http://bowser.macminicolo.net/~jhuckaby/bugs/firefox-ogg1/leak.html

Every refresh of this page results in about 15 MB more memory usage.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These elements are only collected when the cycle collector runs. Are you giving the browser a chance to go idle and perform cycle collection? It should happen if you leave it alone for a few seconds.

If I run these testcases with XPCOM_MEM_LEAK_LOG=1 and shutdown, I don't see any leaks on Mac.

Comment 6

[Jesse Ruderman]

I can still reproduce with the testcase in comment 1, *not* letting the browser go idle or even "finish" with the testcase.  Testing Firefox trunk on Leopard.

Comment 7

[Jesse Ruderman]

I can still reproduce with the testcase in comment 1, *not* letting the browser go idle or even "finish" with the testcase.  Testing Firefox trunk on Leopard.

Comment 8

[Joseph Huckaby]

Robert, yes, you are correct.  If I let the browser sit for a few seconds, THEN refresh, there is no memory leak.  Thanks for catching this, I was being too impatient :)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: create nsContentUtils::Register/UnregisterShutdownObserver

Some simple refactoring, since I'm going to create more shutdown observers.

Jonas, I could punt this review to Mats if you're busy and OK with him reviewing it.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: break MediaLoadListener cycle at shutdown

As described by comments in the code, we create a temporary cycle of references during media loads: after the load listener is created, and before we reach OnStartRequest, the listener references the element, which references the channel, which references the listener. We rely on OnStartRequest to break the cycle, but when we shut down with pending loads, OnStartRequest may not be called.

This patch fixes the bug by making the load listener observe shutdown notifications and break the cycle if one happens.

This patch also makes the media element observe shutdown notifications while it's holding a self-reference, and releases the self-reference if shutdown occurs. (Recall that the self-reference is used to keep the element alive while it's loading; we don't want to GC it if there's still a possibility it might fire events that would trigger listeners.)

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 425336 [details] [diff] [review]
Part 1: create nsContentUtils::Register/UnregisterShutdownObserver

Mmm... code kill..

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/6ede2694f90b
Followup bustage fix:
http://hg.mozilla.org/mozilla-central/rev/7c24dc44ca00