217967 - FF104 crash [@ PL_DHashTableOperate ] changing caps access control prefs

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P1)

Description

[Adam Hauner]

Crash mentioned by Hermann in bug 217463 comment #11.
(Brendan: many of last changes were by you, so CCing)

TB23170439W: 
PL_DHashTableOperate 9fbdf395

Stack Trace:
PL_DHashTableOperate [c:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c line 491]
nsScriptSecurityManager::LookupPolicy
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 1030]
nsScriptSecurityManager::CheckPropertyAccessImpl
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 636]
nsScriptSecurityManager::CheckPropertyAccess
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 485]
nsDOMClassInfo::doCheckPropertyAccess
[c:/builds/seamonkey/mozilla/dom/src/base/nsDOMClassInfo.cpp line 2994]
nsWindowSH::NewResolve
[c:/builds/seamonkey/mozilla/dom/src/base/nsDOMClassInfo.cpp line 4262]
XPC_WN_Helper_NewResolve
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp line
909]
js_LookupProperty [c:/builds/seamonkey/mozilla/js/src/jsobj.c line 2394]
LookupUCProperty [c:/builds/seamonkey/mozilla/js/src/jsapi.c line 2296]
JS_LookupUCProperty [c:/builds/seamonkey/mozilla/js/src/jsapi.c line 2547]
GlobalWindowImpl::GetObjectProperty
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp line 4284]
nsContentTreeOwner::SetStatus
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp line 347]
GlobalWindowImpl::SetStatus
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp line 1508]
GlobalWindowImpl::SetNewDocument
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp line 505]
DocumentViewerImpl::InitInternal
[c:/builds/seamonkey/mozilla/content/base/src/nsDocumentViewer.cpp line 872]
DocumentViewerImpl::Init
[c:/builds/seamonkey/mozilla/content/base/src/nsDocumentViewer.cpp line 648]
nsDocShell::SetupNewViewer
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp line 4814]
nsDocShell::Embed [c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp line
4147]
nsDocShell::CreateContentViewer
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp line 4552]
nsDSURIContentListener::DoContent
[c:/builds/seamonkey/mozilla/docshell/base/nsDSURIContentListener.cpp line 110]
nsDocumentOpenInfo::DispatchContent
[c:/builds/seamonkey/mozilla/uriloader/base/nsURILoader.cpp line 418]
nsDocumentOpenInfo::OnStartRequest
[c:/builds/seamonkey/mozilla/uriloader/base/nsURILoader.cpp line 228]
nsViewSourceChannel::OnStartRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/viewsource/src/nsViewSourceChannel.cpp
line 474]
nsHttpChannel::CallOnStartRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp line 640]
nsHttpChannel::OnStartRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp line 3256]
nsInputStreamPump::OnStateStart
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp line 366]
nsInputStreamPump::OnInputStreamReady
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp line 328]
nsInputStreamReadyEvent::EventHandler
[c:/builds/seamonkey/mozilla/xpcom/io/nsStreamUtils.cpp line 117]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c line 672]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c
line 610]
nsEventQueueImpl::ProcessPendingEvents
[c:/builds/seamonkey/mozilla/xpcom/threads/nsEventQueue.cpp line 395]
0x16007004

Source File :
c:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c line no: 491 
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/pldhash.c#491

Build: 2003082904 CrashDate: 2003-08-29 UptimeMinutes: 41 Total: 41
OS: Windows 98 4.10 build 67766222
URL: http://www.bishopmcdevitt.org/mcdband/
Comment: testing http://bugzilla.mozilla.org/show_bug.cgi?id=217463 Bug 217463
The frame inside is windows character set. The outside page is Unicode. The
Frame is garbage. After clearing cache the website is loaded correctly after
reload it shows


There are other crashes with this signature, but not all has this part:

PL_DHashTableOperate [c:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c line 491]
nsScriptSecurityManager::LookupPolicy
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 1030]
nsScriptSecurityManager::CheckPropertyAccessImpl
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 636]
nsScriptSecurityManager::CheckPropertyAccess
[c:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp line 485] 

http://ftp.mozilla.org/pub/data/crash-data/detailed-crash-analysis-all.html#PL_DHashTableOperate

Last crash bug with same signature was bug 192207 - M130B Trunk crash [@
0x00000000 - PL_DHashTableOperate]

Comment 1

[Brendan Eich [:brendan]]

Don't blame the leaf function.  This looks like fallout from caps changes. 
Reassigning to caillon.

/be

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can someone pinpoint when this started happening?

Comment 3

[Mike Kaply [:mkaply]]

Do you see this crash consistently?

Comment 4

[Hermann Schwab]

I can only talk about my crash, TB23170439W, which Adam reported.
I had about 3 to 5 crashes that day, or a day before or after, when I was
switching profiles, and opened preferences. Sorry that I can´t give you talkback
IDs, but I deinstalled, when I had problems with a nightlie.
I spent a lot of time with this site, changing profiles, trying different cache
options, download source, view source, installed LiveHTTPHeaders, but that was
the only crash I had at this site. I´m scarce with RAM (96MB) and disk space on
this computer, maybe that was the cause for this crash. To me it looked like
nothing special, not related to the action I just was doing.
I´ve also got a lot of tabs open, but I don´t remember this event.
But having the attention here, what do you think of Bug 217463 ?
A png file is served as text/html in <img border=0 alt=""
src="http://gmsgms.freelogs.com/counter/index.php?u=gmsgms&s=bluepl"
ALIGN="middle" BORDER=0 HSPACE=4 VSPACE=2> and Mozillas Autodetect Logic
switches the charset of the frame to UTF-16LE.
Is this invalid, or TechEvangelism only, or also a mozilla bug?
I´m looking for comments, or decisions, in bug 217463.

Comment 5

[Brendan Eich [:brendan]]

Could mOriginToPolicyMap be null when LookupPolicy is called?  If so, dpolicy
will be null when LookupPolicy calls PL_DHashTableOperate if (!cpolicy).

/be

Comment 6

[Hermann Schwab]

TB23302715E Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030903

Had about ten tabs open, clicked on a link in the tenth, and while it was
loading, went to my first, Bug List of todays bugs, fully loaded. Clicked
View->Page Source, immediate crash.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I hit this (or one with the same leaf) a few times/month - anyone I should talk
to on IRC when I experience it next?

Comment 8

[Brendan Eich [:brendan]]

caillon or me.  What was the bad pointer?

/be

Comment 9

[Giorgio Maone [:ma1]]

Dunno if it is the same bug or closely related.

My extension NoScript (http://www.noscript.net) sets default policy to
javascript.enabled=noAccess
and creates a custom policy with javascript.enabled=allAccess.

It allows user to manage a "live" whitelist for JavaScript execution, giving him
an easy and natural UI to add or remove sites from the custom "allAccess" policy.

It appears to lead Firefox to random crashes, among which the following for
which I've got a stack trace.

In this case ops contains bogus values, instead of the right function pointers,
at the istruction

keyHash = table->ops->hashKey(table, key);

 >	xpcom.dll!PL_DHashTableOperate(PLDHashTable * table=0x03364080, const void *
key=0x00bc2d60, PLDHashOperator op=PL_DHASH_LOOKUP)  Riga 490 + 0xd	C
 	caps.dll!nsScriptSecurityManager::LookupPolicy(nsIPrincipal *
aPrincipal=0x03383930, const char * aClassName=0x00bc2d60, long
aProperty=0x02f0397c, unsigned int aAction=0x00000001, ClassPolicy * *
aCachedClassPolicy=0x00000000, SecurityLevel * result=0x0012de30)  Riga 1011 +
0xf	C++
 	caps.dll!nsScriptSecurityManager::CheckPropertyAccessImpl(unsigned int
aAction=0x00000001, nsIXPCNativeCallContext * aCallContext=0x00000000, JSContext
* cx=0x0300f288, JSObject * aJSObject=0x035469d0, nsISupports * aObj=0x00000000,
nsIURI * aTargetURI=0x00000000, nsIClassInfo * aClassInfo=0x00000000, const char
* aClassName=0x00bc2d60, long aProperty=0x02f0397c, void * *
aCachedClassPolicy=0x00000000)  Riga 625 + 0x2d	C++
 	caps.dll!nsScriptSecurityManager::CheckPropertyAccess(JSContext *
cx=0x0300f288, JSObject * aJSObject=0x035469d0, const char *
aClassName=0x00bc2d60, long aProperty=0x02f0397c, unsigned int
aAction=0x00000001)  Riga 483	C++
 	caps.dll!nsScriptSecurityManager::CheckObjectAccess(JSContext * cx=0x0300f288,
JSObject * obj=0x03546688, long id=0x02f0397c, JSAccessMode mode=JSACC_WRITE,
long * vp=0x0012dee4)  Riga 465 + 0x2e	C++
 	js3250.dll!js_InternalGetOrSet(JSContext * cx=0x0300f288, JSObject *
obj=0x03546688, long id=0x02ea3c00, long fval=0x035469d0, JSAccessMode
mode=JSACC_WRITE, unsigned int argc=0x00000001, long * argv=0x03ccdb00, long *
rval=0x03ccdb00)  Riga 1088 + 0x1ab	C
 	js3250.dll!js_SetProperty(JSContext * cx=0x0300f288, JSObject *
obj=0x03546688, long id=0x02ea3c00, long * vp=0x03ccdb00)  Riga 2863 + 0x34	C
 	js3250.dll!JS_SetProperty(JSContext * cx=0x0300f288, JSObject *
obj=0x03546688, const char * name=0x034ae5b8, long * vp=0x03ccdb00)  Riga 2543
+ 0x1b	C
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *
wrapper=0x039d3870, unsigned short methodIndex=0x0006, const nsXPTMethodInfo *
info=0x034ae500, nsXPTCMiniVariant * nativeParams=0x0012e2e8)  Riga 1320 + 0x25	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=0x0006,
const nsXPTMethodInfo * info=0x034ae500, nsXPTCMiniVariant * params=0x0012e2e8)
 Riga 450	C++
 	xpcom.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x039d3870, unsigned int
methodIndex=0x00000006, unsigned int * args=0x0012e3b0, unsigned int *
stackBytesToPop=0x0012e3a0)  Riga 117 + 0x1c	C++
 	xpcom.dll!SharedStub()  Riga 147	C++
 	tkitcmps.dll!nsAutoCompleteController::ClosePopup()  Riga 748	C++
 	tkitcmps.dll!nsAutoCompleteController::SetInput(nsIAutoCompleteInput *
aInput=0x00000000)  Riga 110	C++
 	tkitcmps.dll!nsFormFillController::StopControllingInput()  Riga 971	C++
 	tkitcmps.dll!nsFormFillController::Unload(nsIDOMEvent * aLoadEvent=0x039097f8)
 Riga 808	C++
 	gklayout.dll!DispatchToInterface(nsIDOMEvent * aEvent=0x039097f8,
nsIDOMEventListener * aListener=0x038e125c, unsigned int (nsIDOMEvent *)*
aMethod=0x01864e90, const nsID & aIID={...}, int * aHasInterface=0x0012e5e8) 
Riga 127 + 0xb	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext *
aPresContext=0x03c77cf8, nsEvent * aEvent=0x0012eb90, nsIDOMEvent * *
aDOMEvent=0x0012eb58, nsIDOMEventTarget * aCurrentTarget=0x03bd7ed0, unsigned
int aFlags=0x00000004, nsEventStatus * aEventStatus=0x0012eb8c)  Riga 1524 +
0x23	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsIPresContext *
aPresContext=0x03c77cf8, nsEvent * aEvent=0x0012eb90, nsIDOMEvent * *
aDOMEvent=0x0012eb58, unsigned int aFlags=0x00000004, nsEventStatus *
aEventStatus=0x0012eb8c)  Riga 2841	C++
 	gklayout.dll!nsXULElement::HandleChromeEvent(nsIPresContext *
aPresContext=0x03c77cf8, nsEvent * aEvent=0x0012eb90, nsIDOMEvent * *
aDOMEvent=0x0012eb58, unsigned int aFlags=0x00000004, nsEventStatus *
aEventStatus=0x0012eb8c)  Riga 3988 + 0x23	C++
 	gklayout.dll!GlobalWindowImpl::HandleDOMEvent(nsIPresContext *
aPresContext=0x03c77cf8, nsEvent * aEvent=0x0012eb90, nsIDOMEvent * *
aDOMEvent=0x0012eb58, unsigned int aFlags=0x00000007, nsEventStatus *
aEventStatus=0x0012eb8c)  Riga 916	C++
 	gklayout.dll!DocumentViewerImpl::Unload()  Riga 1108 + 0x23	C++
 	docshell.dll!nsDocShell::FireUnloadNotification()  Riga 825	C++
 	docshell.dll!nsDocShell::CreateContentViewer(const char *
aContentType=0x039c4270, nsIRequest * request=0x03b07328, nsIStreamListener * *
aContentHandler=0x03b5a388)  Riga 4797	C++
 	docshell.dll!nsDSURIContentListener::DoContent(const char *
aContentType=0x039c4270, int aIsContentPreferred=0x00000000, nsIRequest *
request=0x03b07328, nsIStreamListener * * aContentHandler=0x03b5a388, int *
aAbortProcess=0x0012ed08)  Riga 109 + 0x21	C++
 	docshell.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener *
aListener=0x0332f888, nsIChannel * aChannel=0x03b07328)  Riga 724 + 0x42	C++
 	docshell.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest *
request=0x03b07328, nsISupports * aCtxt=0x00000000)  Riga 452 + 0x39	C++
 	docshell.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest *
request=0x03b07328, nsISupports * aCtxt=0x00000000)  Riga 323 + 0x10	C++
 	necko.dll!nsHttpChannel::CallOnStartRequest()  Riga 639 + 0x3c	C++
 	necko.dll!nsHttpChannel::ProcessNormal()  Riga 776 + 0x8	C++
 	necko.dll!nsHttpChannel::ProcessResponse()  Riga 675 + 0x8	C++
 	necko.dll!nsHttpChannel::OnStartRequest(nsIRequest * request=0x03cef9d8,
nsISupports * ctxt=0x00000000)  Riga 3573 + 0xb	C++
 	necko.dll!nsInputStreamPump::OnStateStart()  Riga 377 + 0x2a	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *
stream=0x03b46784)  Riga 333 + 0xb	C++
 	xpcom.dll!nsInputStreamReadyEvent::EventHandler(PLEvent * plevent=0x03c1d804)
 Riga 119	C++
 	xpcom.dll!PL_HandleEvent(PLEvent * self=0x03c1d804)  Riga 673 + 0xa	C
 	xpcom.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x00bd7f88)  Riga 608
+ 0x9	C
 	xpcom.dll!nsEventQueueImpl::ProcessPendingEvents()  Riga 398 + 0xc	C++
 	gkwidget.dll!nsWindow::DispatchPendingEvents()  Riga 3678	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000200, unsigned
int wParam=0x00000000, long lParam=0x033b03c2, long * aRetValue=0x0012f77c) 
Riga 4030	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00d60818, unsigned int
msg=0x00000200, unsigned int wParam=0x00000000, long lParam=0x033b03c2)  Line
1349 + 0x1b	C++
 	USER32.dll!77d18734() 	
 	USER32.dll!77d18816() 	
 	USER32.dll!77d189cd() 	
 	USER32.dll!77d18a10() 	
 	gkwidget.dll!nsAppShell::Run()  Line 135	C++
 	appshell.dll!nsAppShellService::Run()  Line 495	C++
 	firefox.exe!xre_main(int argc=0x00000003, char * * argv=0x003d77c8, const
nsXREAppData * aAppData=0x0041e01c)  Line 1907 + 0x23	C++
 	firefox.exe!main(int argc=0x00000003, char * * argv=0x003d77c8)  Line 58 +
0x12	C++
 	firefox.exe!mainCRTStartup()  Line 400 + 0x11	C
 	kernel32.dll!7c816d4f() 	

Comment 10

[Giorgio Maone [:ma1]]

I've got reports from Linux NoScript users as well (no debug symbols):

#0 0xb7d2bd51 in kill () from /lib/i686/libc.so.6
#1 0xb7e7b2a1 in pthread_kill () from /lib/i686/libpthread.so.0
#2 0xb7e7b60b in raise () from /lib/i686/libpthread.so.0
#3 0x08053a04 in nsProfileLock::FatalSignalHandler ()
#4 0xb7e7e0f7 in __pthread_sighandler () from /lib/i686/libpthread.so.0
#5 <signal handler called>
#6 0xb7f2f9de in PL_DHashTableOperate () from /usr/lib/firefox-1.0.4/libxpcom.so
#7 0xb7a4a6e7 in ?? () from /usr/lib/firefox-1.0.4/components/libcaps.so
#8 0x0820e730 in ?? ()
#9 0xb7597347 in ?? () from /usr/lib/firefox-1.0.4/components/libgklayout.so
#10 0x00000000 in ?? ()

#0 0xb7d2bd51 in kill () from /lib/i686/libc.so.6
#1 0xb7e7b2a1 in pthread_kill () from /lib/i686/libpthread.so.0
#2 0xb7e7b60b in raise () from /lib/i686/libpthread.so.0
#3 0x08053a04 in nsProfileLock::FatalSignalHandler ()
#4 0xb7e7e0f7 in __pthread_sighandler () from /lib/i686/libpthread.so.0
#5 <signal handler called>
#6 0xb7f2f7a7 in PL_DHashTableSetAlphaBounds () from
/usr/lib/firefox-1.0.4/libxpcom.so
#7 0xb7f2fa1e in PL_DHashTableOperate () from /usr/lib/firefox-1.0.4/libxpcom.so
#8 0xb7a4a6e7 in ?? () from /usr/lib/firefox-1.0.4/components/libcaps.so
#9 0x08351748 in ?? ()
#10 0xb7a4d3ab in ?? () from /usr/lib/firefox-1.0.4/components/libcaps.so
#11 0x00000000 in ?? () 

Comment 11

[Jay Patel [:jay]]

This crash has spiked in Firefox 1.0.4 Talkback data after being off the radar
for 1.0.1-1.0.3.  Not sure if this is a regression or something out there is
triggering this crash more often with the latest release.

Here is the lastest Talkback data:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=PL_DHashTableOperate&vendor=MozillaOrg&product=Firefox10&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

And a recent incident:
Incident ID: 6503497
Stack Signature	PL_DHashTableOperate 328fc372
Product ID	Firefox10
Build ID	2005051112
Trigger Time	2005-06-08 13:47:01.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	xpcom.dll + (0000ef95)
URL visited	http://www.shockwave.com/sw/spotlight/dj_vault/?swhomeclicktrack=MUSIC1
User Comments	clicked on a link ("Indian Summer" ~King Kooba) it asked to
download and I clicked ok. Then FireFox crashed and asked to provide feedback if
I wanted. Hopefully you can figure out what happened. Thanks for the great work....
Since Last Crash	104197 sec
Total Uptime	431927 sec
Trigger Reason	Access violation
Source File, Line No.
d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/ds/pldhash.c,
line 490
Stack Trace 	
PL_DHashTableOperate 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/ds/pldhash.c,
line 490]
nsScriptSecurityManager::LookupPolicy 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/caps/src/nsScriptSecurityManager.cpp,
line 1011]
nsScriptSecurityManager::CheckPropertyAccessImpl 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/caps/src/nsScriptSecurityManager.cpp,
line 626]
nsScriptSecurityManager::CheckPropertyAccess 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/caps/src/nsScriptSecurityManager.cpp,
line 483]
nsDOMClassInfo::doCheckPropertyAccess 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/dom/src/base/nsDOMClassInfo.cpp,
line 3172]
nsWindowSH::NewResolve 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/dom/src/base/nsDOMClassInfo.cpp,
line 4703]
XPC_WN_Helper_NewResolve 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1414]
js_LookupPropertyWithFlags 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/jsobj.c,
line 2503]
FindConstructor 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/jsobj.c,
line 1970]
GetClassPrototype 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/jsobj.c,
line 3601]
js_NewObject 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/jsobj.c,
line 1851]
JS_NewObject 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/jsapi.c,
line 2098]
nsXPConnect::InitClassesWithNewWrappedGlobal 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,
line 489]
mozJSComponentLoader::GlobalForLocation 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,
line 1044]
mozJSComponentLoader::ModuleForLocation 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,
line 914]
mozJSComponentLoader::GetFactory 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,
line 461]
nsFactoryEntry::GetFactory 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/components/nsComponentManager.h,
line 292]
nsComponentManagerImpl::CreateInstanceByContractID 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/components/nsComponentManager.cpp,
line 1997]
nsCreateInstanceByContractID::operator() 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/glue/nsComponentManagerUtils.cpp,
line 76]
nsCOMPtr_base::assign_from_helper 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/xpcom/glue/nsCOMPtr.cpp,
line 114]
nsExternalAppHandler::OnStartRequest 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,
line 1649]
nsDocumentOpenInfo::OnStartRequest 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/uriloader/base/nsURILoader.cpp,
line 328]
nsHttpChannel::CallOnStartRequest 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 639]
nsHttpChannel::ProcessNormal 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 777]
nsHttpChannel::ProcessResponse 
[d:/builds/tinderbox/Fx-Aviary1.0.1/WINNT_5.0_Depend/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 713]
nsHttpChannel::AddRef
0xdfb3e904


If people can reproduce this with the latest Aviary branch builds and come up
with a fix, we should consider this for future Firefox 1.0.x releases (since
this is somewhat of a regression).

Comment 12

[Giorgio Maone [:ma1]]

(In reply to comment #11)

> If people can reproduce this with the latest Aviary branch builds and come up
> with a fix, we should consider this for future Firefox 1.0.x releases (since
> this is somewhat of a regression).

Please notice that the stack traces I reported are from 1.0.4.
You can easily reproduce this bug installing and using the forementioned
NoScript extension ( http://www.noscript.net ), which does nothing else than
changing caps preferences (sites list of a particular policy) using JavaScript
code in the UI thread (no fancy threading).

As a side thought, I wonder if Netscape 8 policy features trigger the same
crashes...?

Comment 13

[Peter J Nadler]

noscript crashes firefox 1.04

Comment 14

[Giorgio Maone [:ma1]]

I think I've spotted the problem, but I need more insight from people who know 
the nsPrincipal lifecycle.

It seems to me that, after a permission change,
nsScriptSecurityManager::LookupPolicy() can be still called with an "old"
nsPrincipal which holds a cached reference to an "old" (and likely freed and
corrupted) mSecurityPolicy object. 
This corrupted object is stored as a void *, but it is actually a DomainPolicy:
i.e. the HashTable which is passed to PL_DHashTableOperate(). This method
expects to find a function pointer as a memember of table->ops, but founds
garbage, and here comes the crash - see my comment #9.

I used to believe it was an uninitialized, or a partially initialized hashtable
to cause this problem (fooled by my Java background, I was searching for some
concurrency issue overlooking the monothreaded constraints on
nsScriptSecurityManager).
Tracing with more patience, I've found that the poisoned hashtable is a
previously known and previously valid pointer, which "becomes" invalid after a
CAPS pref change and a consequent nsScriptSecurityManager::InitPolicies() call.
I've also traced these corrupted objects to be always obtained by a successful
call to aPrincipal->GetSecurityPolicy(),
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#926,
i.e. cached rather than actually looked up.

I think somehow SetSecurityPolicy(nsnull) should be called on every principal
whenever InitPolicies() is executed, or at least old nsPrincipal objects should
be prevented from being recycled after InitPolicies().

Probably I've said a lot of silly things, but I'm really not an expert in
Mozilla internals hacking. What do gurus think?

Comment 15

[timeless]

sounds about right

Comment 16

[Giorgio Maone [:ma1]]

Attachment: More consistent DomainPolicy lifecycle management avoids use of corrupted hashtable data

DomainPolicy already adopts a reference counting strategy, but it doesn't use
it in a consistent manner: 

1) nsPrincipal doesn't update reference count of cached policy
2) nsSecurityManager brutally deletes mDefaultPolicy not taking reference count
in account

The two inconstincencies, put together, lead to use of stolen, corrupted
hashtable data taken improperly cached by nsPrincipal, which is the origin of
this bug.

This patch 
1) adds global a static InvalidateAll() and an instance IsInvalid() to
DomainPolicy class
2) adds InvalidateAll() call and mDefaultPolicy reference count update in
nsScriptSecurityManager::InitPolicies() 
3) adds proper reference count update in nsPrincipal.securityPolicy setter
4) adds invalidation check in nsPrincipal.securityPolicy getter (if cached
DomainPolicy is invalid, it will be reference uncounted and eventually
destroyed while nsnull is returned)

This avoids use of cached invalid data and fixes this bug

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 187535 [details] [diff] [review]
More consistent DomainPolicy lifecycle management avoids use of corrupted hashtable data

>+#ifdef CAPS_DEBUG_DomainPolicyLifeCycle

nit: make this DEBUG_CAPS_DomainPolicyLifeCycle

nit: there's a few tabs and two-space indents. Use 4-space indents consistently
to match with the style in these files.

>+// DomainPolicy members
>+#ifdef CAPS_DEBUG_DomainPolicyLifeCycle
>+PRUint32 DomainPolicy::sObjects=0;
>+void DomainPolicy::_printPopulationInfo()
>+{
>+	printf("CAPS.DomainPolicy: Generation %d, %d DomainPolicy objects.\n", 
>+      sGeneration, sObjects);
>+}
>+#endif
>+PRUint32 DomainPolicy::sGeneration = 0;
>+void DomainPolicy::InvalidateAll() 
>+{

a little vertical whitespace wouldn't hurt

You should put InvalidateAll() and IsValid() in the header file so they can be
inlined. Caps is a suck on our JS performance, let's be as kind as possible to
it.

>     mDefaultPolicy = new DomainPolicy();
>     if (!mOriginToPolicyMap || !mDefaultPolicy)
>         return NS_ERROR_OUT_OF_MEMORY;
> 
>+    mDefaultPolicy->Hold();

If you create the DomainPolicy() ok but the map failed your refcounting will be
off because the hold is after the return. Might want to separate out those
checks, but if you do make sure you do all the teardowns before bailing on OOM.

> nsPrincipal::GetSecurityPolicy(void** aSecurityPolicy)
> {
>+  if (mSecurityPolicy &&
>+    NS_REINTERPRET_CAST(DomainPolicy *, mSecurityPolicy)->IsInvalid()
>+    ) 
>+    SetSecurityPolicy(nsnull);
>+  
>   *aSecurityPolicy = mSecurityPolicy;

These casts are ugly enough that it might be worth making mSecurityPolicy a
DomainPolicy* (as long as the signatures stay void** and void* so outsiders
don't need to know about DomainPolicy details).

e.g. SetSecurityPolicy could be

     DomainPolicy* newPolicy = NS_REINTERPRET_CAST(etc);
     if (newPolicy)
       newPolicy->Hold();

     if (mSecurityPolicy)
	mSecurityPolicy->Drop();

     mSecurityPolicy = newPolicy;

r- to force a new patch for these minor fixups, but looks like the right idea

Comment 18

[Daniel Veditz [:dveditz]]

Nominating because changes in 1.0.4 shot this into topcrash ranks.

Comment 19

[Giorgio Maone [:ma1]]

Attachment: Supersedes 187535 as requested by reviewer + one more fix

It complies to the inlining, formatting, allocation error handling and casting
requirements listed by dveditz.

It also moves the mPolicyPrefsChanged dirty flag reset, from the top, to the
(successful) bottom of the nsScriptSecurityManager::InitPolicies() method.

IMHO, doing so prevents potential security issues under memory pressure
depicted by the following scenario:
1) Memory pressure causes an allocation, e.g. new mDomainPolicy(), to fail
after mPolicyPrefsChanged has already been set to PR_FALSE;
2) Current LookupPolicy() execution, which called InitPolicy() because
mPolicyPrefsChanged was PR_TRUE, correctly fails propagating the error
3) Next LookupPolicy execution, though, will not retry to call InitPolicy,
because mPolicyPrefsChanged now is PR_FALSE

As a result of the above events, we end with uninitialized or half-initialized
policy settings (e.g. mDefaultPolicy=nsnull and/or mOriginToPolicyMap=nsull)
which are treated as valid and can provide security levels which are lower than
the ones user believes to have set.

Resetting mPolicyPrefs just before a successful return from InitPolicies()
guarantees that LookupPolicy() will fail fast until valid policy settings can
be fully configured.

Comment 20

[Giorgio Maone [:ma1]]

Attachment: Supersedes 187638 with nsPrincipal.h diff and a (void *) cast

nsPrincipal.h has slipped out from my previous diff, sorry.
I've also taken the chance to add a (void *) cast which may make some compiler
happier :-)

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 187640 [details] [diff] [review]
Supersedes 187638 with nsPrincipal.h diff and a (void *) cast

r=dveditz -- thanks!

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187640 [details] [diff] [review]
Supersedes 187638 with nsPrincipal.h diff and a (void *) cast

>+    //-- Initialize a new mOriginToPolicyMap
>     mOriginToPolicyMap =
>       new nsObjectHashtable(nsnull, nsnull, DeleteDomainEntry, nsnull);
>+    if (!mOriginToPolicyMap)
>+        return NS_ERROR_OUT_OF_MEMORY;
> 
>-    //-- Reset and initialize the default policy
>-    delete mDefaultPolicy;
>+    //-- Create, refcount and initialize a new default policy 
>     mDefaultPolicy = new DomainPolicy();
>-    if (!mOriginToPolicyMap || !mDefaultPolicy)
>+    if (!mDefaultPolicy)
>         return NS_ERROR_OUT_OF_MEMORY;

This looks like we leak mOriginToPolicyMap if we fail to allocate
mDefaultPolicy.

>+    mDefaultPolicy->Hold();
>     if (!mDefaultPolicy->Init())
>         return NS_ERROR_UNEXPECTED;

And this looks like we leak both if we fail to initialize mDefaultPolicy.

Is there some magic refcounting thing going on that I don't see, or?

Other than that, looks good.  sr=shaver with fixes to those error leaks.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 187640 [details] [diff] [review]
Supersedes 187638 with nsPrincipal.h diff and a (void *) cast

a=dveditz for branches. Please add "fixed1.7.9" and "fixed-aviary1.0.5" when
landed on the appropriate branch. When you've got trunk approval and land there
then it can be resolved FIXED.

Comment 24

[Giorgio Maone [:ma1]]

(in reply to comment #22 by shaver)
This is how the "critical" code looks like after applying this patch (with more
comments to address your leak concerns):

<snip>

    //-- Clear mOriginToPolicyMap: delete mapped DomainEntry items,
    //-- whose dtor decrements refcount of stored DomainPolicy object
    delete mOriginToPolicyMap;

// *** here we have no policy map and an invalid pointer   

    //-- Marks all the survivor DomainPolicy objects (those cached
    //-- by nsPrincipal objects) as invalid: they will be released
    //-- on first nsPrincipal::GetSecurityPolicy() attempt.
    DomainPolicy::InvalidateAll();
    
    //-- Release old default policy
    if(mDefaultPolicy)
        mDefaultPolicy->Drop();

// *** here we *may* have destroyed mDefaultPolicy, 
// *** unless it is cached in some nsPrincipal - in this case, 
// *** it will be destroyed as soon as the holding nsPrincipal(s) 
// *** are used or destroyed
    
    //-- Initialize a new mOriginToPolicyMap
    mOriginToPolicyMap =
      new nsObjectHashtable(nsnull, nsnull, DeleteDomainEntry, nsnull);

/*** here we may have an origin2policy map or null
/*** if the following test is true, we return leaving no leak behind
/*** (aside cached invalid policies that will be 
/***  deleted in a near future)
    if (!mOriginToPolicyMap)
        return NS_ERROR_OUT_OF_MEMORY;


    //-- Create, refcount and initialize a new default policy 
    mDefaultPolicy = new DomainPolicy();

/*** if the following test is true, we return leaving our 
/*** mOriginToPolicyMap behind. It is empty, and it
/*** will be destroyed on next InitPolicies() call
/*** that will happen very soon (next LookupPolicy
/*** attempt) because we're leaving
/*** mSecurityPolicyPrefChanged set to PR_TRUE

    if (!mDefaultPolicy)
        return NS_ERROR_OUT_OF_MEMORY;

    mDefaultPolicy->Hold();

/*** From now on, if we fail we leave a refcounted +1 
/*** mDefaultPolicy and our policy map behind.
/*** They both will be destroyed on next InitPolicies()
/*** call, that will be very soon (next LookupPolicy
/*** attempt) because we're leaving
/*** mSecurityPolicyPrefChanged set to PR_TRUE

    if (!mDefaultPolicy->Init())
        return NS_ERROR_UNEXPECTED;
</snip>

To summarize, after each LookupPolicy() we're guaranteed to have one 
mOriginToPolicyMap at most and one mDefaultPolicy at most (aside invalid default
policy objects cached by nsPrincipal objects, which will be released as soon as
they are accessed or their nsPrincipal holder is deleted).
Considering that nsScriptSecurityManager *is a singleton*, I believe we can
afford holding one mDefaultPolicy and one map until the next LookupPolicy() and
not calling it a "leak" :-)

Comment 25

[Giorgio Maone [:ma1]]

Patch landed by timeless on both branches and trunk.
Thank you all :-)

Comment 26

[Jay Patel [:jay]]

updating flags to reflect that this landed on the branches

Comment 27

[Frank Wein [:mcsmurf]]

Might it be that this patch here has caused a topcrasher (#1 for SeaMonkey and
also some crashes are turning up for Firefox)? See
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=domainpolicy%3A%3Adrop&vendor=All&product=All&platform=All&sortby=bbid
i suspect this checkin here because the crashes started showing up since 2005/06/30
probably someone should file a new bug on this issue...

Comment 28

[Giorgio Maone [:ma1]]

(In reply to comment #27)
> probably someone should file a new bug on this issue...

bug 300853 - I'll hopefully fix it tomorrow morning (2:21 A.M. now in my timezone)