480851 - ASSERTION: "You can't dereference a NULL nsCOMPtr with operator->()" when xpconnect checks thread bits after threads are gone [@ DEBUG_CheckWrapperThreadSafety]

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: gdb stack on shutdown

Running the test from bug 479490 comment 0 hangs (crashes) the browser on exit. Using a debug build of Minefield I get the above assertion. It cannot reproduce it on 1.9.1 so far.

Steps:
1. Open http://mozilla.pettay.fi/xhr_upload/xhr_upload_demo_sync.html
2. Click on some button at the bottom to load those pages
3. Click on 'new XmlHTTPRequest'
4. Wait for the resulting alert message
5. Exit the browser

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../../dist/include/xpcom/nsCOMPtr.h, line 796

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x112267f8 in DEBUG_CheckWrapperThreadSafety (wrapper=0x1da4a010) at /data/mozilla/build/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:3390
3390	    else if(NS_SUCCEEDED(wrapper->mThread->IsOnCurrentThread(&val)) && !val)

#0  0x112267f8 in DEBUG_CheckWrapperThreadSafety (wrapper=0x1da4a010) at /data/mozilla/build/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:3390
#1  0x111e5600 in XPCCallContext::XPCCallContext (this=0xbfff99e0, callerLanguage=XPCContext::LANG_JS, cx=0x842e00, obj=0x127b3000, funobj=0x0, name=310071756, argc=4294967295, argv=0x0, rval=0x0) at /data/mozilla/build/mozilla-central/mozilla/js/src/xpconnect/src/xpccallcontext.cpp:154

More frames are given by the attachment.

Comment 1

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

Debug only, and note jsd on the stack. You using firebug?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, seems like Firebug causes this assertion. Running with a profile where Firebug isn't installed I cannot see this assertion. Adding John to the CC list.

Comment 3

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

I'm changing that testcase all the time (may have changed it already).
Henrik, can you reproduce the crash with the current version?
If yes, then I'll copy it to somewhere and start modifying only the
copy.

Comment 4

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

The only way mThread could be null is if do_GetCurrentThread fails, and it looks like that could only realistically happen if the wrapper is created after XPCOM has locked down the service manager at shutdown. What is firebug doing in its 'script destroyed' hook?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> If yes, then I'll copy it to somewhere and start modifying only the
> copy.

Yeah, I can reproduce it with the latest version. Please attach it here as testcase or place it on a safe place. Thanks.

Comment 6

[John J. Barton]

(In reply to comment #4)
> What is firebug doing in
> its 'script destroyed' hook?

Which version of firebug?

http://code.google.com/p/fbug/source/browse/branches/firebug1.4/components/firebug-service.js#1557

Maybe the hook is not being removed before shutdown.

Install the tracing version, 1.4X, open Firebug icon menu tracing window, set
Options FBS_ERRORS, and pref extensions.firebug-tracing-service.toOSConsole
true.

You should see the shutdown messages on window.dump(). Maybe it will give some
hints.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: gdb stack (load web page)

Oh, my fault. Somehow this profile had installed Firebug 1.2.1. I've upgraded to Firebug 1.4Xa12 now.

When opening the given web page I get a lot of assertions and the program quits with a sigtrap.

###!!! ASSERTION: Inner window detected in Equality hook!: 'win->IsOuterWindow()', file /data/mozilla/build/mozilla-central/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 6673
###!!! ASSERTION: Inner window detected in Equality hook!: 'other->IsOuterWindow()', file /data/mozilla/build/mozilla-central/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 6679

#0  JS_Assert (s=0x1bcc5c78 "0", file=0x1bcc5f5c "/data/mozilla/build/mozilla-central/mozilla/js/jsd/jsd_scpt.c", ln=806) at /data/mozilla/build/mozilla-central/mozilla/js/src/jsutil.cpp:62
#1  0x1bcb4564 in jsd_ClearExecutionHook (jsdc=0x1a8cf3d0, jsdscript=0x1dd25090, pc=500310864) at /data/mozilla/build/mozilla-central/mozilla/js/jsd/jsd_scpt.c:806
#2  0x1bcaf016 in JSD_ClearExecutionHook (jsdc=0x1a8cf3d0, jsdscript=0x1dd25090, pc=500310864) at /data/mozilla/build/mozilla-central/mozilla/js/jsd/jsdebug.c:533
#3  0x1bcba656 in jsdScript::ClearBreakpoint (this=0x1dd25890, aPC=0) at /data/mozilla/build/mozilla-central/mozilla/js/jsd/jsd_xpc.cpp:1433
#4  0x004be4eb in NS_InvokeByIndex_P (that=0x1dd25890, methodIndex=30, paramCount=1, params=0xbfffb464) at /data/mozilla/build/mozilla-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179

Comment 8

[timeless]

bent: it hardly matters, xpconnect still has to work at shutdown...

Comment 9

[Karsten Düsterloh]

I ran into the actual crash here (xpccallcontext.cpp:155) with the fix for bug 364864 - in debug builds, it dies on shutdown while trying to QI a JS component...
I can provide Linux gdb stacks, if someone feels they're helpful.

Comment 10

[Karsten Düsterloh]

Attachment: comment out crashing check

Can't we just remove the offending line in xpccallcontext.cpp:

197        DEBUG_CheckWrapperThreadSafety(mWrapper);

It's not used in releases, only in debug builds and test boxes.
It doesn't lead to an error code you could deal with but crashes the whole application. 
That's not particularily useful, given that patches get rejected due to failing tests...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 405669 [details] [diff] [review]
comment out crashing check

> It doesn't lead to an error code you could deal
> with but crashes the whole application. 

Yes, this is normal and expected behavior for assertion failures.

> That's not particularily useful,
> given that patches get rejected due to failing
tests...

If patches are being rejected due to failing assertions in a debug build (which is what we're talking about here), then they _should_ have been rejected, unless you can prove the assertion is asserting something that just doesn't have to be true.

In this case, it sounds like at the very least firebug is running various script after XPCOM shutdown and probably shouldn't be doing that, no?  Has someone tried fixing the "unregister the hooks on shutdown" bug in jsd and seeing how that affects things here?

Comment 12

[John J. Barton]

(In reply to comment #11)
...
> In this case, it sounds like at the very least firebug is running various
> script after XPCOM shutdown and probably shouldn't be doing that, no?  Has
> someone tried fixing the "unregister the hooks on shutdown" bug in jsd and
> seeing how that affects things here?

We don't normally crash on exit in Firebug, so I'm not sure what scripts you think we are running. We try to unhook on quit application granted:
http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#2851
As you can see there we have tracing for this so the procedure from comment #6 will determine if the shutdown code is run before the crash.

Comment 13

[timeless]

I have a patch for this which is slowly winding through try server. Please note that this crash has been reported before in bug 364864. Hopefully patch will be up in an hour or so.

Comment 14

[timeless]

Attachment: stop playing cop when xpcom stops befriending

this teaches nsXPConnect about when xpcom has shut down threading. From here, xpconnect can stop trying to enforce rules it can't understand.

Comment 15

[timeless]

Comment on attachment 411738 [details] [diff] [review]
stop playing cop when xpcom stops befriending

MozillaTry objects. I need to figure out what went wrong.

Comment 16

[:Gijs (he/him)]

(In reply to comment #15)
> (From update of attachment 411738 [details] [diff] [review])
> MozillaTry objects. I need to figure out what went wrong.

Did you ever figure this out? I don't suppose the logs are still around, as it's been 3 weeks? I'd like to get some traction on this, for the sake of bug 364864 :-(.

Comment 17

[Johnny Stenback (:jst)]

Not blocking 1.9.3 on this unless this starts to show up in large numbers in the wild.

Comment 18

[Karsten Düsterloh]

Attachment: watch xpcom-shutdown-threads, as proposed

This is a working version of timeless' attachment #411738 [details] [diff] [review], against trunk. 

I also threw this + patch 364864 + test 364864 onto the tryserver twice (http://hg.mozilla.org/try/pushloghtml?changeset=1ba09ac00827 and http://hg.mozilla.org/try/pushloghtml?changeset=8fb27d137da1) which doesn't seem to be unhappy (apart from random unrelated problems, if I don't misread the tinderboxpushlog), hence requesting review.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 449554 [details] [diff] [review]
watch xpcom-shutdown-threads, as proposed

1)  Since the observer service holds a strong ref to nsXPConnect with this patch, removing in the dtor makes no sense.  Removing in the thread shutdown notification would be more sensible.

2)  The static member should be sThreadsGone.

3)  We should set sThreadsGone to PR_FALSE at the point when we add our observer.

Comment 20

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.