nsDirectoryService implements nsIProperties, which looks like some kind of interface to stick random stuff on an object. This appears to have been the case since the implementation of nsDirectoryService back in 2000 by one Doug Turner. As khuey points out, it is a bad idea for a service to have a strong reference, but that's basically the only thing this does. If we're lucky, nobody besides content/base/test/test_bug345339.html actually uses this.

I don't understand what this bug is about. The whole point of the directory service is to implement nsIProperties. What exactly are you planning on removing? Also what does "it is a bad idea for a service to have a strong reference" mean?

In the other bug I said that either things with process lifetime should not have strong references to things with more transitory lifetimes (e.g. DOM objects) or they need to manually drop those references at an appropriate time.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > I don't understand what this bug is about. The whole point of the directory > service is to implement nsIProperties. What exactly are you planning on > removing? There's a test ( content/baase/test/test_bug345339.html ) that sticks an nsIFile into nsDirectoryService. Since the WebIDLification of File, this causes a shutdown leak on e10s in the child process. I'm trying to fix the leak. Maybe the right fix is to modify the test so it removes the file from the directory service thing and just hope nobody else actually leaking everything like this? (I haven't had a chance to look to closely at what nsDirectoryService is actually trying to do.)

I don't understand what the relationship is between sticking an nsIFile into the nsDirectoryService (which is the whole point of the directory service) and the webidlification of File (which should have very little to do with nsIFile). In general, tests shouldn't be changing state beyond their lifetime, so this test should probably be removing its file property as it cleans up. But even without that this shouldn't be causing a shutdown leak. The directory service should be properly cleaned up near the end of XPCOM shutdown.

Are you saying that there's strong references from an nsIFile to a DOM File? That does not sound right to me. If that really is the case then that's a bug. File certainly can hold a reference to nsIFile objects, but the reverse sounds unexpected.

> File certainly can hold a reference to nsIFile objects, but the reverse > sounds unexpected. We don't have such reference from nsIFile to DOM File.

Sorry, I don't actually know what is going on in this test yet. It does create an iframe with an InputElement, so that's presumably where the dom::File comes in. nsDirectoryService may not actually be related after all. I'll get some more information.

The nsGlobalWindow here is being rooted by a dom::File. The File has a refcount of 3. There are two known edges in the CC graph, both from HTMLInputElements. Based on DMD heap scanning, it looks like the third reference is from an HTMLInputElementState allocated inside of HTMLInputElement::SaveState(). Based on reading the code, it looks like this gets stuck on an nsPresState: nsPresState* state = GetPrimaryPresState(); if (state) { state->SetStateProperty(inputState); } Reading some more code, it looks like we have a strong reference from nsDocument::mLayoutHistoryState to nsLayoutHistoryState::mStates to nsPresState to HTMLInputElementState to HTMLInputElement. nsLayoutHistoryState, nsPresState and HTMLInputElement state aren't cycle collected, but offhand it looks like they could be. Does that sounds like the right fix here or should the cycle be broken manually here somehow?

Hmm, looking at the actual heap scan I got, it looks like the nsLayoutHistoryState is actually owned by an nsSHEntryShared, which is owned by an nsSHEntry, so cycle collecting stuff won't help, because it is really owned by doc shell stuff.

Ah, it makes sense that session-history is holding on Blobs, which in turn can (as of recently) hold on to Windows. I'm not sure why HTMLInputElementState holds on to Blobs rather than just filenames. Right now it wouldn't make a difference functionality-wise since we always initiate Blobs in an HTMLInputElement from filenames. However at some point in a not too distant future we'll hopefully allow assigning Blobs into a HTMLInputElement, at which point that won't work. So I think the simplest long-term solution here would be to make HTMLInputElementState hold on to FileImpls instead of Files. I.e. * Make HTMLInputElementState::mFiles into an nsTArray<nsRefPtr<FileImpl>> * Call File::Impl() on each blob passed to HTMLInputElementState::SetFiles * Make HTMLInputElementState::GetFiles take an nsTArray<nsRefPtr<File>> and an nsISupports parent. * Make Make HTMLInputElementState::GetFiles create new Files by calling this constructor http://mxr.mozilla.org/mozilla-central/source/dom/base/File.h#135

Does that mean that putting a Blob into HTMLInputElement and pulling it out again would yield objects that aren't ===?

No. This code isn't used during normal assignments. It's used if you leave the page, navigate far enough away from it that it gets destroyed, and then the user navigates back to it. But at that point no objects maintain identity.

I'm not sure I'll be able to implement a fix for this in the next week or two.

I have investigated this issue a bit and it turned out that the problem is with nsLayoutHistoryState. In HTMLInputElement::SaveState() we save the state as a HTMLInputElementState object. This has an array of File objects but they are not CCed at all.

Well, from my investigation in comment 9, it looks like non-CCed nsSH stuff owns the nsLayoutHistoryState, so CCing the nsLayoutHistoryState won't fix the leak.

Comment on attachment 8520715 [details] [diff] [review] leak.patch > case VALUE_MODE_FILENAME: > { >- const nsTArray<nsRefPtr<File>>& files = inputState->GetFiles(); >+ const nsTArray<nsRefPtr<FileImpl>>& fileImpls = inputState->GetFileImpls(); >+ nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject(); Could we assert that we have global.

So, what that patch does, is to do not use DOMFile in the sessionHistory but just the FileImpl. File objects are CCed but they are just a shell around FileImpl objects. Using FileImpl instead File fixes this issue. https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0b98ccc319

Thanks for fixing this. It should probably get backported to 35 (and 36 I guess?) because while the original test case was on e10s, I don't see any reason it wouldn't also affect non-e10s.