542437 - Leak nsGlobalWindow with onunload, <video> in <title>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Dunno if this is related to bug 503989.

Comment 1

[Jesse Ruderman]

I noticed this because it can entrain nsDocShell.

Comment 2

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

Must be a cycle somewhere. I don't see where though.

Comment 3

[Johnny Stenback (:jst)]

Given that this is something that sites are very unlikely to intentionally run into, I won't be blocking the release on fixing this leak.

Comment 4

[Jesse Ruderman]

jst, I think roc nominated this bug based on it making it hard to look for *other* leak bugs, not based on the worry that users would hit this one.

Comment 5

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

The main thing I'm concerned about is that this leak might be a deeper problem and not just media-related.

Comment 6

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

I've tracked it down to nsDocument::DoNotifyPossibleTitleChange being called after nsDocument::Destroy has been called on that document. That calls NS_GetContentList which puts a list in gCachedContentList *after* nsContentList::OnDocumentDestroy has been called on the document, so the list hangs around in gCachedContentList indefinitely, causing the leak.

nsBaseContentList::Shutdown would release gCachedContentList, but that never runs because nsLayoutStatics::Shutdown only runs if all nsGlobalWindows are released, and a window has been entrained somehow. I'm not sure how, though.

Comment 7

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

I can easily work around it by having nsDocument::DoNotifyPossibleTitleChange return early if mIsGoingAway, but that doesn't seem like quite the right fix. For one thing, there might be other code that creates a content list for the document. Or we might create a content list for a document that's never belonged to a viewer and won't have nsDocument::Destroy called.

We need to do something about gCachedContentList... perhaps the right thing to do is to actually store one cached list per document and traverse it in cycle collection?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

There is an existing bug about gCachedContentList, bug 523083.
Peterv wanted some other approach to that bug though.

Comment 9

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

Right. The patch I prepared for this bug is a lot like your patch in bug 523083.

Comment 10

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

Fixed by checkin for bug 523083.