414977 - insufficient unlink methods in some DOM classes?

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack for destructor of the nsGlobalWindow in previous log

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.

Comment 3

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

Ben, do you have the cycles to look at this one?

Comment 4

[Damon Sicore (:damons)]

bent, given that you are off the cocoa exceptions now, just wanted to make sure Jonas' question above is answered.   You on this?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yup, spent the last half of today on it.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Fix warning, v1 [checked in]

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.

Comment 7

[Peter Van der Beken [:peterv]]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304809 [details] [diff] [review]
Fix warning, v1 [checked in]

r=dbaron

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Teach the warning about XPCWrappedNatives, v1

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why can't these objects be freed sooner?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Teach the warning about XPCWrappedNatives, v1

Oops, I messed up the existence test while cleaning up this patch. This one works.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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...

Comment 14

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

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...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 16

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

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 17

[Peter Van der Beken [:peterv]]

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

We've got a better approach to fix this.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Collect XPCWrappedNatives earlier

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.

Comment 19

[Peter Van der Beken [: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.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Collect XPCWrappedNatives earlier, v2 [checked in]

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.

Comment 21

[Johnny Stenback (:jst)]

Upping priority to P1, Ben wants this to bake in b4 if at all possible.

Comment 22

[Johnny Stenback (:jst)]

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.

Comment 23

[Mike Schroepfer]

Comment on attachment 306331 [details] [diff] [review]
Collect XPCWrappedNatives earlier, v2 [checked in]

Let's get this in tonight for nightly baking

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Fixed?

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Fix for xpc_hacker's [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).

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And with that I'm going to call this fixed. Other insufficient unlink warnings should be filed as separate bugs.