937612 - Null crash [@ nsContentUtils::IsSystemPrincipal] on null sSecurityManager

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

Happened to me during run of network xpcshell tests (the crashed one was test_seer.js).

Today gum (cache2 on) with some local patches (I believe unrelated), based on m-c@361a24da7112.

sSecurityManager = 0x00000000

>	xul.dll!nsContentUtils::IsSystemPrincipal(nsIPrincipal * aPrincipal=0x02c66af0)  Line 4438 + 0xe bytes	C++
 	xul.dll!nsPermissionManager::TestExactPermissionFromPrincipal(nsIPrincipal * aPrincipal=0x02c66af0, const char * aType=0x13d552c0, unsigned int * aPermission=0x0033f664)  Line 949 + 0x9 bytes	C++
 	xul.dll!nsSiteSecurityService::IsSecureURI(unsigned int aType=0, nsIURI * aURI=0x03a3c7d0, unsigned int aFlags=0, bool * aResult=0x0033f7a3)  Line 477 + 0x37 bytes	C++
 	xul.dll!nsHttpHandler::SpeculativeConnect(nsIURI * aURI=0x03a3c7d0, nsIInterfaceRequestor * aCallbacks=0x03ac044c)  Line 1800 + 0x1c bytes	C++
 	xul.dll!IOServiceProxyCallback::OnProxyAvailable(nsICancelable * request=0x03b2d2c0, nsIURI * aURI=0x03a3c7d0, nsIProxyInfo * pi=0x00000000, tag_nsresult status=NS_OK)  Line 1225	C++
 	xul.dll!nsAsyncResolveRequest::DoCallback()  Line 222	C++
 	xul.dll!nsAsyncResolveRequest::OnQueryComplete(tag_nsresult status=NS_OK, const nsCString & pacString={...}, const nsCString & newPACURL={...})  Line 193	C++
 	xul.dll!ExecuteCallback::Run()  Line 85	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0033fa43)  Line 610 + 0x19 bytes	C++
 	xul.dll!NS_ProcessPendingEvents(nsIThread * thread=0x01f290e0, unsigned int timeout=4294967295)  Line 201 + 0x14 bytes	C++
 	xul.dll!mozilla::ShutdownXPCOM(nsIServiceManager * servMgr=0x00000000)  Line 663	C++
 	xul.dll!NS_ShutdownXPCOM(nsIServiceManager * servMgr=0x00000000)  Line 618 + 0x9 bytes	C++
 	xul.dll!XRE_XPCShellMain(int argc=23, char * * argv=0x01f120f4, char * * envp=0x01ec31b8)  Line 1587 + 0x7 bytes	C++
 	xpcshell.exe!NS_internal_main(int argc=30, char * * argv=0x01f120d8, char * * envp=0x01ec31b8)  Line 45 + 0x12 bytes	C++
 	xpcshell.exe!wmain(int argc=30, wchar_t * * argv=0x01f11778)  Line 109 + 0x16 bytes	C++
 	xpcshell.exe!__tmainCRTStartup()  Line 552 + 0x19 bytes	C
 	xpcshell.exe!wmainCRTStartup()  Line 371	C


Or is this nsContentUtils bug with missing sInitialized check?

Comment 1

[(no longer active)]

nsContentUtils::Shutdown() clears out sSecurityManager, and this code runs after nsContentUtils::Shutdown() as far as I can tell, so the crash is quite expected.  ;-)

Shouldn't we avoid calling SpeculativeConnect during shutdown?

Comment 2

[Patrick McManus [:mcmanus]]

ehasan - do you think profile-change-net-teardown and/or xpcom-shutdown have been observed?

Comment 3

[(no longer active)]

xpcom-shutdown is dispatched here: <http://dxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#656> and NS_ProcessPendingEvents() is called right after that (line 662), so yes to both (https://wiki.mozilla.org/XPCOM_Shutdown).

Comment 4

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/try/rev/e222ce34ff58

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 831241 [details] [diff] [review]
patch 0

Review of attachment 831241 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab.

I have confirmed that nsContentUtils::Shutdown cannot be called sooner then from NS_XPCOM_SHUTDOWN_OBSERVER_ID where nsLayoutStatics::Release() (->? nsLayoutStatics::Shutdown() -> nsContentUtils::Shutdown()) is called (at nsLayoutModule.cpp).

However, in a rare case when some observer added between LayoutShutdownObserver and nsHttpHandler (and this is really often used observer topic!) loops the event queue, we can still potentially crash...

So, not a 100% correct fix.

Comment 7

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/044eb80709a5

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/044eb80709a5

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 831241 [details] [diff] [review]
patch 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  long standing bug, but 881804 (ff27) will make it more prominent
User impact if declined: crash risk during shutdown
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): extremely low
String or IDL/UUID changes made by this patch: none

Its a crash bug, so its good to backport a safe fix at least to the one release that is likely to trigger it (ff27)

Comment 10

[bhavana bajaj [:bajaj]]

Comment on attachment 831241 [details] [diff] [review]
patch 0

low risk, null check helping fix a crash. Looks good to land

Comment 11

[Patrick McManus [:mcmanus]]

  https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf07c05aace

Comment 12

[u279076]

Honza, are you able to reproduce this anymore?

Comment 13

[Honza Bambas (:mayhemer)]

No.

Comment 14

[u279076]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> No.

Thanks!