411531 - Assertion failure: !rt->gcRunning, a clear case of what not to do in object finalizer

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

Found with a linux trunk debug build from today. I don't have a good test case for this, but found it while investigating crashes on ancestry.com. 

igor>	File a bug against xpconnect: this is the clear case of what not to do in object finalizer.

<bc>	i don't have a good testcase though :-(

<igor>	it does not matter, the trace is too explicit.

To reproduce, go to ancestry and click around the public pages. If you are lazy like me, use Spider to spider the public pages.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: stack

Comment 2

[Jesse Ruderman]

The stack trace in bug 402966 also shows nsHTMLImageElement::~nsHTMLImageElement doing lots of stuff.  Is Igor saying that nsHTMLImageElement::~nsHTMLImageElement should be changed even if the patch to cycle collector in bug 402966 fixes the crash?

Comment 3

[Igor Bukanov]

The difference of this bug and bug 402966 is that here the destructor of nsHTMLImageElement is called from a JS finalizer while in the bug 402966 the problem happens when the cycle collector calls the destructor. I am not yet sure that the patch in bug 402966 covers this.

Comment 4

[Peter Van der Beken [:peterv]]

This is unrelated to cycle collection. I don't see anyone calling SetDeferReleases anymore, so we're not deferring releases till after GC ever.

Comment 5

[Peter Van der Beken [:peterv]]

Looks like a regression from bug 409433. Benjamin, did you really want to remove deferReleasesUntilAfterGarbageCollection? I bet that'll cause a lot of bugs like this one, and is unrelated to running GC on the main thread only.

Comment 6

[Benjamin Smedberg]

I meant to *always* defer releases until after GC... is that not what I accomplished?

Comment 7

[Peter Van der Beken [:peterv]]

Nope :-). You probably want to change mDeferReleases(JS_FALSE) in the XPCJSRuntime constructor or remove mDeferReleases/GetDeferReleases/SetDeferReleases altogether.

Comment 8

[Benjamin Smedberg]

Grr yes, fix coming up.

Comment 9

[Benjamin Smedberg]

Attachment: Really always defer releases, rev. 1

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 296346 [details] [diff] [review]
Really always defer releases, rev. 1

>Index: js/src/xpconnect/src/xpcjsruntime.cpp
>===================================================================

>@@ -789,17 +789,16 @@ JSBool XPCJSRuntime::GCCallback(JSContex
>             }
>             case JSGC_END:
>             {
>                 // NOTE that this event happens outside of the gc lock in
>                 // the js engine. So this could be simultaneous with the
>                 // events above.
> 
>                 // Do any deferred released of native objects.
>-                if(self->GetDeferReleases())
>                 {

Please remove the braces and reindent.

>Index: js/src/xpconnect/src/xpcwrappednative.cpp
>===================================================================

>@@ -659,17 +659,17 @@ XPCWrappedNative::~XPCWrappedNative()

>+        if(rt && rt->GetDoingFinalization())
>         {
>             if(!rt->DeferredRelease(mIdentity))
>             {

Combine the two ifs and reindent.

>@@ -1000,17 +1000,17 @@ XPCWrappedNative::FlatJSObjectFinalized(

>+                if(rt)
>                 {
>                     if(!rt->DeferredRelease(obj))
>                     {

Same here.

Comment 11

[John Gaunt (redfive)]

Attachment: new patch with notes handled

points addressed from comment #10.

I hope this isn't rude, but I'd really like to get this patch in right away, Songbird is blocked from updating our XULRunner by this one.

Comment 12

[John Gaunt (redfive)]

Comment on attachment 296365 [details] [diff] [review]
new patch with notes handled

crap, this was created against an old pull, doesn't applly and doens't catch all the calls of GetDeferRelease()

Comment 13

[John Gaunt (redfive)]

Attachment: patched against the tip this time - addresses c#10 notes

This one is better, it even builds. :-)

Comment 14

[John Gaunt (redfive)]

Hmmm, I crash now in the while loop. Line 816 is releasing XPCComponents, but it's mUtils member is garbage (frame 5). Not sure if this is just a bad build or fallout from this patch.

802                     while(1)
803                     {
804                         nsISupports* obj;
805                         {
806                             PRInt32 count = array->Count();
807                             if(!count)
808                             {
809                                 array->Compact();
810                                 break;
811                             }
812                             obj = reinterpret_cast<nsISupports*>
813                                                   (array->ElementAt(count-1));
814                             array->RemoveElementAt(count-1);
815                         }
816                         NS_RELEASE(obj);
817                     }

#0  0x00000035c3c97581 in nanosleep () from /lib64/libc.so.6
#1  0x00000035c3c973a4 in sleep () from /lib64/libc.so.6
#2  0x00002aaaac78957c in ah_crap_handler (signum=11) at nsSigHandlers.cpp:149
#3  0x00002aaaac7a0d91 in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:210
#4  <signal handler called>
#5  0x00002aaaac7b48db in nsXPCComponents::ClearMembers (this=0x756ab0)
    at /builds/xulrunner/trunk/mozilla/js/src/xpconnect/src/xpccomponents.cpp:3838
#6  0x00002aaaac7b4a4c in ~nsXPCComponents (this=0x756ab0)
    at /builds/xulrunner/trunk/mozilla/js/src/xpconnect/src/xpccomponents.cpp:3824
#7  0x00002aaaac7bcaa4 in nsXPCComponents::Release (this=0x756ab0)
    at /builds/xulrunner/trunk/mozilla/js/src/xpconnect/src/xpccomponents.cpp:3709
#8  0x00002aaaac7dcc20 in XPCJSRuntime::GCCallback (cx=0x741380, status=JSGC_END)
    at /builds/xulrunner/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:816
#9  0x00002aaaacf32f23 in DOMGCCallback (cx=0x741380, status=JSGC_END)
    at /builds/xulrunner/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:3459
#10 0x00002aaaac082359 in js_GC (cx=0x741380, gckind=GC_NORMAL) at /builds/xulrunner/trunk/mozilla/js/src/jsgc.c:2743

Comment 15

[John Gaunt (redfive)]

Attachment: full stack of crash with this patch on xulrunner

This is the stack I get now, it crashes very shortly after component registration. Does this raise any flags for anyone?

Comment 16

[Benjamin Smedberg]

Comment on attachment 296369 [details] [diff] [review]
patched against the tip this time - addresses c#10 notes

peterv (& jgaunt), you can't combine the if clauses in xpcwrappednative.cpp like this because you end up with a double-release in the "else" clause. I will commit the first version with the reindentation.

Comment 17

[John Gaunt (redfive)]

(In reply to comment #16)
> (From update of attachment 296369 [details] [diff] [review])
> peterv (& jgaunt), you can't combine the if clauses in xpcwrappednative.cpp
> like this because you end up with a double-release in the "else" clause. I will
> commit the first version with the reindentation.
> 

Yeah caught that after a little poking around, didn't look closely enough at what I was changing. Thanks for taking a look.

Comment 18

[John Gaunt (redfive)]

One more thing, I was looking at that logic and it seems like it can be combined and cleaned up a bit, at least to get rid of the duplicate ->release and RELEASE statements (is there a reason they are different?).

Here's what I came up with, take it or leave it. Your patch worked great for me and I'm happy with the fix, just thought a little clean up might be worthwhile. (just the diff for the one piece of the whole patch)

RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v
retrieving revision 1.174
diff -u -p -r1.174 xpcwrappednative.cpp
--- js/src/xpconnect/src/xpcwrappednative.cpp   10 Jan 2008 14:10:03 -0000      1.174
+++ js/src/xpconnect/src/xpcwrappednative.cpp   11 Jan 2008 01:47:26 -0000
@@ -664,17 +664,11 @@ XPCWrappedNative::~XPCWrappedNative()
     if(mIdentity)
     {
         XPCJSRuntime* rt = GetRuntime();
-        if(rt && rt->GetDeferReleases() && rt->GetDoingFinalization())
-        {
-            if(!rt->DeferredRelease(mIdentity))
-            {
-                NS_WARNING("Failed to append object for deferred release.");
-                // XXX do we really want to do this???
-                NS_RELEASE(mIdentity);
-            }
-        }
-        else
+        if(!rt || 
+           !rt->GetDoingFinalization() ||
+           !rt->DeferredRelease(mIdentity))
         {
+            NS_WARNING("Failed to append object for deferred release.");
             NS_RELEASE(mIdentity);
         }
     }
@@ -1005,17 +999,9 @@ XPCWrappedNative::FlatJSObjectFinalized(
                 NS_ASSERTION(*(int*)obj != 0,          "bad pointer!");
 #endif
                 XPCJSRuntime* rt = GetRuntime();
-                if(rt && rt->GetDeferReleases())
-                {
-                    if(!rt->DeferredRelease(obj))
-                    {
-                        NS_WARNING("Failed to append object for deferred release.");
-                        // XXX do we really want to do this???
-                        obj->Release();
-                    }
-                }
-                else
+                if(!rt || !rt->DeferredRelease(obj))
                 {
+                    NS_WARNING("Failed to append object for deferred release.");
                     obj->Release();
                 }
                 to->SetNative(nsnull);

Comment 19

[Benjamin Smedberg]

Fixed on trunk.

Comment 20

[Bob Clary [:bc] (inactive)]

I tried a number of things on ancestry.com with a current trunk debug build on linux and couldn't crash. Marking verified.