Closed Bug 503989 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

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

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase)

Attachments

(3 files)

No description provided.
Summary: Pending <audio src> loads leak on shutdown (entraining nsDocument shutdown leak of nsDocument → Pending <audio src> loads leak on shutdown (entraining nsDocument)
Attached file testcase
Blocks: 343943
Keywords: mlk, testcase
This prevents me from looking for other nsDocument leak bugs :(
Duplicate of this bug: 541753
Assignee: nobody → roc
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.
blocking2.0: --- → ?
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.
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.
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.
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 :)
Blocks: 475285
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.
Attachment #425336 - Flags: review?(jonas)
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.)
Attachment #425338 - Flags: review?(chris.double)
Whiteboard: [needs review]
Comment on attachment 425336 [details] [diff] [review]
Part 1: create nsContentUtils::Register/UnregisterShutdownObserver

Mmm... code kill..
Attachment #425336 - Flags: review?(jonas) → review+
Attachment #425338 - Flags: review?(chris.double) → review+
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/6ede2694f90b
Followup bustage fix:
http://hg.mozilla.org/mozilla-central/rev/7c24dc44ca00
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
blocking2.0: ? → beta1
You need to log in before you can comment on or make changes to this bug.