117386 - Crash changing themes [@ nsXBLPrototypeResources::FlushSkinSheets]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Blake Ross]

Yes, I am a useless bug reporter. I don't know what I did. But I do know that I 
crashed twice with it sometime today, in today's nightly, doing something.

nsXBLPrototypeResources::FlushSkinSheets 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLPrototypeResources.cpp, line 
149] 
nsXBLPrototypeBinding::FlushSkinSheets 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLPrototypeBinding.cpp, line 
432] 
FlushScopedSkinSheets 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLDocumentInfo.cpp, line 352] 
_hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, line 199] 
PL_HashTableEnumerateEntries [../../../lib/ds/plhash.c, line 430] 
nsHashtable::Enumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, 
line 362] 
nsXBLDocumentInfo::FlushSkinStylesheets 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLDocumentInfo.cpp, line 360] 
FlushScopedSkinStylesheets 
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULPrototypeCache.cpp, 
line 399] 
_hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, line 199] 
PL_HashTableEnumerateEntries [../../../lib/ds/plhash.c, line 430] 
nsHashtable::Enumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, 
line 362] 
nsXULPrototypeCache::FlushSkinFiles 
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULPrototypeCache.cpp, 
line 422] 
nsChromeRegistry::FlushCaches 
[d:\builds\seamonkey\mozilla\rdf\chrome\src\nsChromeRegistry.cpp, line 1180] 
nsChromeRegistry::LoadProfileDataSource 
[d:\builds\seamonkey\mozilla\rdf\chrome\src\nsChromeRegistry.cpp, line 2849] 
nsChromeRegistry::Observe 
[d:\builds\seamonkey\mozilla\rdf\chrome\src\nsChromeRegistry.cpp, line 3201] 
nsObserverService::NotifyObservers 
[d:\builds\seamonkey\mozilla\xpcom\ds\nsObserverService.cpp, line 214] 
nsProfile::SetCurrentProfile 
[d:\builds\seamonkey\mozilla\profile\src\nsProfile.cpp, line 1198] 
nsProfile::StartApprunner 
[d:\builds\seamonkey\mozilla\profile\src\nsProfile.cpp, line 1868] 
XPTC_InvokeByIndex 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, 
line 106] 
XPCWrappedNative::CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 
2011] 
XPC_WN_CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp, 
line 1267] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 834] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2799] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 850] 
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 925] 
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3407] 
nsJSContext::CallEventHandler 
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1014] 
nsJSEventListener::HandleEvent 
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 182] 
nsEventListenerManager::HandleEventSubType 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, 
line 1206] 
nsEventListenerManager::HandleEvent 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, 
line 1376] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3449] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3468] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3468] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3468] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3468] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3468] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6061] 
PresShell::HandleEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6031] 
nsEventStateManager::CheckForAndDispatchClick 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 
2465] 
nsEventStateManager::PostHandleEvent 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 
1548] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6082] 
PresShell::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5986] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 387] 
nsViewManager::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1930] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83] 
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 850] 
nsWindow::DispatchWindowEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 867] 
nsWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4422] 
ChildWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4672] 
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3381] 
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1112] 
USER32.DLL + 0x3eb0 (0x77e13eb0) 
USER32.DLL + 0x401a (0x77e1401a) 
USER32.DLL + 0x92da (0x77e192da) 
nsWebShellWindow::ShowModal 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsWebShellWindow.cpp, line 1097] 
nsContentTreeOwner::ShowAsModal 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsContentTreeOwner.cpp, line 
408] 
nsWindowWatcher::OpenWindowJS 
[d:\builds\seamonkey\mozilla\embedding\components\windowwatcher\src\nsWindowWatc
her.cpp, line 700] 
nsWindowWatcher::OpenWindow 
[d:\builds\seamonkey\mozilla\embedding\components\windowwatcher\src\nsWindowWatc
her.cpp, line 452] 
nsProfile::LoadDefaultProfileDir 
[d:\builds\seamonkey\mozilla\profile\src\nsProfile.cpp, line 575] 
nsProfile::StartupWithArgs 
[d:\builds\seamonkey\mozilla\profile\src\nsProfile.cpp, line 420] 
nsAppShellService::DoProfileStartup 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 239] 
netscp6.exe + 0x2862 (0x00402862) 
netscp6.exe + 0x162d (0x0040162d) 
netscp6.exe + 0x121c (0x0040121c)

Comment 1

[Blake Ross]

Okay, there have been a lot of occurrences of this judging from talkback data.  
Appears to be when changing themes.  There are a few bugs on crashes when 
changing theme, but I couldn't find any with this stack.

Comment 2

[timeless]

I have it.

here's my guess:
nsXBLPrototypeResources::FlushSkinSheets() line 148 + 41 bytes
    newSheet->GetStyleRuleProcessor(*getter_AddRefs(processor), prevProcessor);
newSheet is null

there are two codepaths to set newSheet
loader->LoadAgentSheet(uri, *getter_AddRefs(newSheet), complete, nsnull);
newSheet = oldSheet;

the condition is if (IsChromeURI(uri))
+ mStr 0x048e6a40 "chrome://calendar/skin/unifinder/unifinder.css"
my uri appears complete.

A quick LXR search indidcates that LoadAgentSheet can fail. Patch coming

Comment 3

[timeless]

Attachment: null check for failed css load

Comment 4

[David Hyatt]

The set of agent sheets doesn't change when you switch skins (or pick a
profile).  It should not be possible for LoadAgentSheet to fail in this code
path.  If it is failing, something else is wrong, and we need to find out what
that is.

Comment 5

[David Bradley]

I think the failure test should come after the call loader->LoadAgentSheet. If
some later added code that set rv to something besides success for this, it
would cause this test to fail erroneously. Also do you still want to do the
assignment of mStyleSheet = newSheets in the error condition? So I'd probably
just do your if after the loader->LoadAgentSheet and if fails or null pointer do
return rv;

Comment 6

[David Bradley]

Comment on attachment 63195 [details] [diff] [review]
null check for failed css load

r=dbradley after setting rv in else of the if (IsChromURI(uri)) test.

My second comment was erroneous, as I missed the newSheet vs newSheets.

Comment 7

[timeless]

Attachment: use continue, don't set rv needlessly

Comment 8

[David Bradley]

Comment on attachment 65127 [details] [diff] [review]
use continue, don't set rv needlessly

r=dbradley

Comment 9

[timeless]

alecf would you sr?

Comment 10

[timeless]

Attachment: really don't store rv. and since we continue we don't need to null check later.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 65199 [details] [diff] [review]
really don't store rv. and since we continue we don't need to null check later.

sr=[e-mail], and I think dbradley's r= should carry over, in case
he's gone to bed -- I say go ahead and get this in for 0.9.8.

/be

Comment 12

[timeless]

fixed, thanks

Comment 13

[Hixie (not reading bugmail)]

Were hyatt's comments ignored?

Comment 14

[timeless]

no. i explained to him how this could happen. it appears that the function 
might be misnamed, because the style sheet being loaded is a chrome app style 
sheet which didn't exist in the other skin [developing a bleeding classic skin 
for calendar allowed me to crash things in interesting ways :) ].