80589 - NS_NewLocalStore can return success but null result - M09 & Trunk crash [@ nsServiceManagerImpl::GetService]

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

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

Caveat: I haven't actually seen this in the debugger, but it's the only way I
can interpret the data in the crash log, and there's certainly a bug here.

In NS_NewLocalStore, we only fill aResult through the impl->QueryReference call,
which is in turn only called if impl->Refresh is called.

If impl->Refresh fails, then we're going to return the success code of the
observer-service NS_WITH_SERVICE, which is likely true.

I bet what happened is that whoever added the profile-switch observer stuff
didn't realize that the code counted on falling through and returning the error
from Refresh, in the case that it failed.

The patch is left as an exercise to one of the readers, though I think switching
to use of a comptr to hold impl's ref and bailing early if impl->Refresh fails
is a decent plan.

Comment 1

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

(Adding crash keyword.  Tip o' the hat to timeless for helping me analyze this.)

Comment 2

[Jaime Rodriguez, Jr.]

Jpatel/Namachi - Per PDT, can you look for NS_NewLocalStore in crash database,
to see if we can find reports on it?

Comment 3

[Jay Patel [:jay]]

I don't see any crashes in Talkback data that have the NS_NewLocalStore stack 
signature.  Has anyone been able to reproduce a crash related to this problem 
and get a stack trace?  That might help find the right stack signature to search 
Talkback for.

Comment 4

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

This bug was found by analyzing the crash data pointed to by the URL associated 
with this bug report.  What more do you need?

Comment 5

[Jay Patel [:jay]]

There have been a few crashes reported by Talkback with the
nsServiceManagerImpl::GetService stack signature in recent builds: 2 crashes
with build 2001052213, 2 with build 2001052115, 1 with build 2001051806 and a 8
crashes reported with build 2001051710.  Here is the info for the latest crash:

Incident ID 30830296 
 Trigger Time 
                2001-05-23 16:04:01 
 User Comments 
                Launching on startup after loading a new theme. 
 Build ID
                2001052213  
 Platform ID
                Win32 
 Stack Trace

nsServiceManagerImpl::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp, line 368] 
nsServiceManager::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp, line 560] 
nsXULDocument::Init
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULDocument.cpp, line 3749] 
NS_NewXULDocument
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULDocument.cpp, line 529] 
nsContentFactory::CreateInstance
[d:\builds\seamonkey\mozilla\content\build\nsContentFactory.cpp, line 570] 
nsComponentManagerImpl::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp, line 1206] 
nsComponentManager::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsRepository.cpp, line 82] 
nsLayoutDLF::CreateRDFDocument
[d:\builds\seamonkey\mozilla\layout\build\nsLayoutDLF.cpp, line 442] 
nsLayoutDLF::CreateRDFDocument
[d:\builds\seamonkey\mozilla\layout\build\nsLayoutDLF.cpp, line 468] 
nsLayoutDLF::CreateInstance
[d:\builds\seamonkey\mozilla\layout\build\nsLayoutDLF.cpp, line 316] 
nsDocShell::NewContentViewerObj
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 3464] 
nsDocShell::CreateContentViewer
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 3365] 
nsDSURIContentListener::DoContent
[d:\builds\seamonkey\mozilla\docshell\base\nsDSURIContentListener.cpp, line 120] 
nsDocumentOpenInfo::DispatchContent
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 353] 
nsDocumentOpenInfo::OnStartRequest
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 220] 
nsJARChannel::OnStartRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\jar\src\nsJARChannel.cpp, line 566] 
nsOnStartRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsRequestObserverProxy.cpp, line 109] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 591] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 524] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1072] 
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 418] 
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1100] 
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1394] 
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1412] 
WinMainCRTStartup() 
KERNEL32.DLL + 0x192a6 (0x77e992a6) 

And here are some comments:
- crash on startup
- Trying to start by clicking on Mozilla.exe
- startup, sometime after the splash before the browser appears
- started the browser

Comment 6

[Chris Waterson]

Attachment: integrate Refresh() and observer setup into Init()

Comment 7

[Chris Waterson]

If you go back to r1.10 of this file, I see that rjc added
|impl->Refresh(PR_TRUE)| to NS_NewLocalStore(). I should've cleaned this up when
I changed the signature of NS_NewLocalStore(), but didn't. This probably set the
bad precedent which ccarlen just followed when he added the profile switching
stuff. Anyway, the above patch moves both things to the Init() method, and
hard-fails if we can't |Refresh(PR_TRUE)|.

Comment 8

[Alec Flett]

r=alecf, very straight forward

Comment 9

[Chris Waterson]

hyatt sez, sr=hyatt! really!

Comment 10

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1

Comment 11

[Chris Waterson]

Fix checked in.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 74765 has been marked as a duplicate of this bug. ***

Comment 13

[Jay Patel [:jay]]

verified fixed.  this crash no longer shows up in Talkback data.

Comment 14

[Jay Patel [:jay]]

Sorry for the spam, adding M09 to summary since bug 74765 was marked a dup.