Closed Bug 414977 Opened 12 years ago Closed 12 years ago

insufficient unlink methods in some DOM classes?

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: dbaron, Assigned: bent.mozilla)

References

Details

(Keywords: memory-leak)

Attachments

(5 files, 3 obsolete files)

Some DOM classes seem to have insufficient unlink methods such that we don't free them when the cycle collector tries to.

Steps to reproduce:
 1. apply attachment 300506 [details] [diff] [review] (from bug 414972) and build with DEBUG_CC defined
 2. follow the steps in bug 413447 comment 18

*Some* of the time I see a bunch of warnings output indicating insufficient unlink methods.
Flags: blocking1.9?
Attached file warning output
This shows the warning output that shows this bug.  Note that I sometimes see the problem in bug 413447 comment 18 without seeing these warnings, whereas sometimes these warnings show up as well.

Even when these warnings happen, I don't see any warnings about things not collected due to missing calls to suspect or unlink.
From XPCOM_MEM_ALLOC_LOG=alloc.log and XPCOM_MEM_LOG_CLASSES=nsGlobalWindow, here's the stack of the destructor of the window mentioned in the above log, which isn't very interesting or informative.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
Ben, do you have the cycles to look at this one?
Assignee: nobody → bent.mozilla
bent, given that you are off the cocoa exceptions now, just wanted to make sure Jonas' question above is answered.   You on this?
Yup, spent the last half of today on it.
Status: NEW → ASSIGNED
So at least half of our problem was that the warning wasn't properly accounting for the deferred release of native objects. This moves the warning after deferred releases have taken place and I'm now only seeing one or two warnings, all with nsGenericElement.
Attachment #304809 - Flags: superreview?(peterv)
Attachment #304809 - Flags: review?(dbaron)
Comment on attachment 304809 [details] [diff] [review]
Fix warning, v1 [checked in]

>@@ -2190,16 +2174,33 @@ nsCycleCollector::Collect(PRUint32 aTryC
>         if (mRuntimes[nsIProgrammingLanguage::JAVASCRIPT]) {
>             collected = static_cast<nsCycleCollectionJSRuntime*>
>                 (mRuntimes[nsIProgrammingLanguage::JAVASCRIPT])->Collect();
>         }
>         else {
>             collected = BeginCollection() && FinishCollection();
>         }
> 
>+#ifdef DEBUG_CC

Maybe add a comment here that we need to do this after the call to (mRuntimes[nsIProgrammingLanguage::JAVASCRIPT])->Collect() because some of the white objects might be kept alive after FinishCycleCollection but be released before (mRuntimes[nsIProgrammingLanguage::JAVASCRIPT])->Collect() returns.
Attachment #304809 - Flags: superreview?(peterv) → superreview+
Comment on attachment 304809 [details] [diff] [review]
Fix warning, v1 [checked in]

r=dbaron
Attachment #304809 - Flags: review?(dbaron) → review+
Comment on attachment 304809 [details] [diff] [review]
Fix warning, v1 [checked in]

I checked in the fix for the warning so no one else wanders down that rabbit hole. Still a little more work to do here though.
Attachment #304809 - Attachment description: Fix warning, v1 → Fix warning, v1 [checked in]
Turns out that the warning also doesn't know about XPCWrappedNatives that are about to be released. JS GC ignores the wrapped JS object if the XPCWrappedNative has a refcount == 2, but if that XPCWrappedNative is owned by an object that's about to be unlinked it then takes another JS GC to properly collect it. We were warning in those circumstances before. This patch does DEBUG_CC only stuff to identify this situation and avoid the unnecessary warning.
Attachment #305122 - Flags: review?(peterv)
Why can't these objects be freed sooner?
Oops, I messed up the existence test while cleaning up this patch. This one works.
Attachment #305122 - Attachment is obsolete: true
Attachment #305139 - Flags: review?(peterv)
Attachment #305122 - Flags: review?(peterv)
(In reply to comment #11)
> Why can't these objects be freed sooner?

So we were failing to unlink an nsHTMLImageElement, created here:

http://www.mozilla.org/__utm.js, line 142 or 147 (I can't remember which).

jst figured out that the 'onload' attribute (an expando property) was causing the element to get put into the mContentWrapperHash of nsDocument to keep the element alive, but it doesn't actually get inserted into the document anywhere. The document in question is cycle-collected before Destroy is ever called on it so there's no way that I know of that we can clear the hash before JS mark occurs.

I guess I don't yet understand why Destroy is not called on this document...
We're very much doing what we tell websites and extensions not to do, stick random junk in global arrays/hashes even though it's not needed any more.

I wonder if we could detect the cases where the only reason an element is being kept alive is because it's wrapper lives in that hash...
So I thought we preserved the invariant that an element can only be in the mContentWrapperHash of its GetOwnerDoc().  Is that not the case?  We do traverse that link using NS_IMPL_CYCLE_COLLECTION_TRAVERSE_PRESERVED_WRAPPER.  Otherwise, what document's mContentWrapperHash is it in?

Or is what's happening that the only thing causing the preserved wrapper reference to go away is that the document is cycle collected?  So if somebody stuck a property on the image pointing to the document, we'd have a leak, since the image otherwise isn't being collected?
Never mind what I said in comment 14. We do collect nodes if the only thing that keep them alive is the wrapper in the hash. This is because we traverse the wrapper in the hash from the element. Very cool indeed.

Comment on attachment 305139 [details] [diff] [review]
Teach the warning about XPCWrappedNatives, v1

We've got a better approach to fix this.
Attachment #305139 - Attachment is obsolete: true
Attachment #305139 - Flags: review?(peterv)
Target Milestone: mozilla1.9beta4 → mozilla1.9
This is a patch that jst and I worked out this afternoon to make XPCWrappedNatives die in one collection instead of two. It works by changing XPConnect to no longer mark the wrapped native if the cycle collector tries to unlink it. Unlink therefore sets a flag on XPCWrappedNative (another bit in mMaybeScope/mMaybeProto) to indicate that the wrapper is unlinking. Also, as the last release is now coming from C++ instead of JS, the FlatJSObjectFinalized and destructor had to be changed around a little to properly destruct.
Attachment #306214 - Flags: review?(peterv)
Comment on attachment 306214 [details] [diff] [review]
Collect XPCWrappedNatives earlier

>diff -r 68c1ba3037e1 js/src/xpconnect/src/xpcprivate.h

>+    GetScope() const
>+        {return (HasProto() && GetProto()) ? GetProto()->GetScope() :

Can't this just check for GetProto? I don't understand why we need to check for null proto here, or why we wouldn't need to check in a bunch of other places.

>diff -r 68c1ba3037e1 js/src/xpconnect/src/xpcwrappednative.cpp

>@@ -655,10 +664,14 @@ XPCWrappedNative::~XPCWrappedNative()

>+    XPCWrappedNativeScope *scope = GetScope();
>+    if(scope)
>+    {
>+        Native2WrappedNativeMap* map = scope->GetWrappedNativeMap();
>+        {   // scoped lock

Don't need to scope this lock, it's scoped by the if now.
Attachment #306214 - Flags: review?(peterv) → review+
Same patch with peterv's comments addressed. Replaced the (HasProto() && GetProto()) with just GetProto() and removed the extra scoping braces. Oh, I also made ExpireWrapper a private function - no one should be calling that except through the cycle collector.
Attachment #306214 - Attachment is obsolete: true
Attachment #306331 - Flags: superreview?(jst)
Attachment #306331 - Flags: review+
Flags: tracking1.9+ → blocking1.9+
Priority: P1 → P2
Upping priority to P1, Ben wants this to bake in b4 if at all possible.
Priority: P2 → P1
Comment on attachment 306331 [details] [diff] [review]
Collect XPCWrappedNatives earlier, v2 [checked in]

Baking this leak fix in a beta would be worth while, especially as it changes XPCWrappedNative destruction order etc, which isn't exactly straight forward.
Attachment #306331 - Flags: superreview?(jst)
Attachment #306331 - Flags: superreview+
Attachment #306331 - Flags: approval1.9b4?
Comment on attachment 306331 [details] [diff] [review]
Collect XPCWrappedNatives earlier, v2 [checked in]

Let's get this in tonight for nightly baking
Attachment #306331 - Flags: approval1.9b4? → approval1.9b4+
Comment on attachment 306331 [details] [diff] [review]
Collect XPCWrappedNatives earlier, v2 [checked in]

XPCWrappedNative changes landed for beta 4.

Not sure if there is more to do here, leaving open for the moment.
Attachment #306331 - Attachment description: Collect XPCWrappedNatives earlier, v2 → Collect XPCWrappedNatives earlier, v2 [checked in]
I'm going to land this, r+sr+a=jst, which fixes some assertions on DEBUG_xpc_hacker builds (not part of the normal build).
Attachment #307142 - Attachment description: Fix for xpc_hacker's → Fix for xpc_hacker's [checked in]
And with that I'm going to call this fixed. Other insufficient unlink warnings should be filed as separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.