234910 - crash exiting mozilla after installing xpi [@ nsXPInstallManager::~nsXPInstallManager()]

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P1)

Description

[Dan M]

Using a Gecko/20040218 build.
Run Mozilla. Install an XPI. (I used Tab Extensions. Tobias Fischer used Message
ID Finder. See bug 233953 comment 12, 13). Exit Mozilla. It crashes. Note this
looks a lot like bug 233953 (verified fixed) but this one is different. It was
probably uncovered when 233953 was fixed.

The crash happens at os->RemoveObserver here:

nsXPInstallManager::~nsXPInstallManager()
{
    nsCOMPtr<nsIObserverService>
os(do_GetService("@mozilla.org/observer-service;1"));
    os->RemoveObserver(this, XPI_PROGRESS_TOPIC);
 
    if (mTriggers)
        delete mTriggers;
}

note in the stack trace that we're in the destructor for the observer service.

nsXPInstallManager::~nsXPInstallManager() line 111 + 47 bytes
nsXPInstallManager::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsXPInstallManager::Release(nsXPInstallManager * const 0x03184d28) line 125 +
144 bytes
nsSupportsArray::Clear(nsSupportsArray * const 0x03184f48) line 559 + 54 bytes
nsSupportsArray::DeleteArray() line 304
nsSupportsArray::~nsSupportsArray() line 147
nsSupportsArray::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsSupportsArray::Release(nsSupportsArray * const 0x03184f48) line 238 + 142 bytes
nsCOMPtr<nsISupportsArray>::~nsCOMPtr<nsISupportsArray>() line 477
nsObserverList::~nsObserverList() line 58 + 11 bytes
nsObserverList::`scalar deleting destructor'(unsigned int 1) + 15 bytes
ReleaseObserverList(nsHashKey * 0x03184ef0, void * 0x03184c58, void *
0x00000000) line 106 + 28 bytes
hashEnumerateRemove(PLDHashTable * 0x00a44f58, PLDHashEntryHdr * 0x00a892d0,
unsigned int 3, void * 0x0012fdc4) line 315 + 26 bytes
PL_DHashTableEnumerate(PLDHashTable * 0x00a44f58, int (PLDHashTable *,
PLDHashEntryHdr *, unsigned int, void *)* 0x1001af10
hashEnumerateRemove(PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *),
void * 0x0012fdc4) line 619 + 34 bytes
nsHashtable::Reset(int (nsHashKey *, void *, void *)* 0x100165c0
ReleaseObserverList(nsHashKey *, void *, void *), void * 0x00000000) line 336 +
21 bytes
nsObjectHashtable::Reset() line 778
nsObjectHashtable::~nsObjectHashtable() line 737
nsObjectHashtable::`vector deleting destructor'(unsigned int 1) + 81 bytes
nsObserverService::~nsObserverService() line 81 + 33 bytes
nsObserverService::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsObserverService::Release(nsObserverService * const 0x00a44ef8) line 71 + 139 bytes
nsCOMPtr_base::assign_assuming_AddRef(nsISupports * 0x00000000) line 431
nsCOMPtr_base::assign_with_AddRef(nsISupports * 0x00000000) line 73
nsCOMPtr<nsISupports>::operator=(nsISupports * 0x00000000) line 807
FreeServiceContractIDEntryEnumerate(PLDHashTable * 0x002af5f4, PLDHashEntryHdr *
0x00a71d38, unsigned int 788, void * 0x00000000) line 2046
PL_DHashTableEnumerate(PLDHashTable * 0x002af5f4, int (PLDHashTable *,
PLDHashEntryHdr *, unsigned int, void *)* 0x1005f620
FreeServiceContractIDEntryEnumerate(PLDHashTable *, PLDHashEntryHdr *, unsigned
int, void *), void * 0x00000000) line 619 + 34 bytes
nsComponentManagerImpl::FreeServices() line 2058 + 19 bytes
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 746

Ben checked in the change that references the Observer Service in
XPInstallManager's dtor on 20040211. Assigning to him.

Comment 1

[Dan M]

I didn't mean to leave Vincent Lefevre uncredited on this bug. He and Tobias
found it; I'm really just verifying it. This does appear nearly identical to bug
234299, except, interestingly, the very end of the stack trace. (And of course
both looked a lot like bug 233953 until that one was cleared out.)

Comment 2

[Robert Accettura [:raccettura]]

Could this be due to the Extensions manager Checkin by Ben Goodger earlier today
(or yesterday I forget)?

Comment 3

[Tobias Fischer]

(In reply to comment #2)
> Could this be due to the Extensions manager Checkin by Ben Goodger earlier today
> (or yesterday I forget)?

No, I don't think so. I have had this crash also with the 20040215 Builds after
installing different *.xpi Extensions (calendar, mnenhy, messageIDF). Later
today I can look for an (old) crash-stack or install this Versions again to
reproduce if needed. 
Sorry, I am no developer, please can tell me someone (PM is ok) which parts from
the stack are needed to report?

Comment 4

[Dan M]

Robert: The stack trace in the initial comment clearly inculpates code checked
in on the 11th.

Tobias: I suspect the stack trace in bug 234299 is the same, though I'm confused
about frames 6 and 7. This crash is well described, and doesn't look like the
kind of error that could result in wildly different crash stacks from the same
root cause, unlike bug 233953. Your crash's stack trace would be worth posting
if it's different. Though if it is I'd be inclined to think you'd found a third
crash, worthy of yet another bug report.

Comment 5

[Tobias Fischer]

Attachment: Stack-Back-Trace from DrWatson

I have added two stack-traces from crash after installing MIDF.xpi and exit
Mozilla. Have tried different xpis, stack-trace is the same.
First one from Build 2004021716, think without the Fix for 233953, second is
stack-trace from Build 2004021913, looks just the same I've posted first for
233953 #12. 

And its nearly the same then Vincents Linux-Stack in 234299. I dont think this
here is another Bug.

Comment 6

[Travis Chase]

*** This bug has been marked as a duplicate of 232501 ***

Comment 7

[Travis Chase]

Sorry for the spam, accidently entered wrong bug number

Comment 8

[Travis Chase]

Sorry for more spam, entering right bug #

*** This bug has been marked as a duplicate of 231969 ***

Comment 9

[Robert Kaiser]

Travis: please don't dupe a bug with lots of informmmation about what's
happening to a bug that has almost no info, even if it was filed earlier.
People who can fix it want those stack traces. They're really helpful, but they
probably won't look into the dupe.

Sorry for even more spam, I'm duping it the other way round.

Comment 10

[Robert Kaiser]

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

Comment 11

[Andrew Schultz]

crash also occurs with linux trunk 20030305

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: null check

If the observer service is gone (which it is in the crash case), there's no
point in removing the XPInstallManager's observer, since it's well... gone.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 143449 [details] [diff] [review]
null check

dbaron told me about a better way to do this.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: better patch

Make XPInstallManager implement nsSupportsWeakReference since it's not used as
a service and can go away early.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 143454 [details] [diff] [review]
better patch

erm. ignore this one.

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: better patch

same comment as before - xpinstall manager implements nsSupportsWeakReference

Comment 17

[chris hofmann]

dan, can you review?  we need to get this in quickly for 1.7b..

Comment 18

[Dan M]

Comment on attachment 143455 [details] [diff] [review]
better patch

Good patch; keeps the install manager from needlessly hanging on until the end
of the program. And does indeed fix the crash. Regardless of the crash, this
seems an improvement.

Still I'd feel safer if you also checked for a null nsIObserverService pointer
before using it. Sure it *shouldn't* ever happen, but... Are you certain it
can't? Is there nothing I can do to bring on that circumstance? What if I start
closing windows during the install, trying to shut the app down?

I tried. Couldn't make it happen. Feeling of unease persists. Either way,
r=danm.

Comment 19

[Dan M]

Oh. Chris, was it me you were asking? You really want DanV's review. But, not
being cc:ed, he wouldn't have noticed your request comment. I assumed you were
asking me. If not, my apologies, and feel free to un+ the patch.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 143455 [details] [diff] [review]
better patch

r/sr=dveditz

I'm not really happy that you made each install manager a global observer
(previously) and I'd like a failure check for the get services, but this stops
the crash pretty cleanly so r=/sr=dveditz as needed

Comment 21

[Bradley Chapman (not reading bugmail, still gone but not forever)]

When will this land? Ben said in 229600 that fixing this will also fix that bug,
which has been biting me in the ass ever since the new extensions installer was
added to Firefox.

Comment 22

[chris hofmann]

I didn't crash after installing extenstions and shutting down with yesterday's
seamonkey build for the first time in a while...   this seems strange because I
don't see a bug number in bonsia showing that this fix has gone in.....


at any rate, we need to make sure this is fixed quickly so we can get 1.7b out
the door and in testing...

Comment 23

[Karsten Düsterloh]

FWIW: I installed an addon into 2004-03-12-08 and it *did* crash... (Win2k)

Comment 24

[chris hofmann]

maybe I wasn't paying close enough attention.  just tried again, and did crash.

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

Dan, I explained to you in bug 227796 why nsXPInstallManager needed to register
as a global observer - this is because other application components need to be
able to send messages to active XPInstall operations, components which may not
have direct access to the Manager instance itself, e.g.

http://lxr.mozilla.org/mozilla/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1229
http://lxr.mozilla.org/mozilla/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1309

Comment 26

[chris hofmann]

Comment on attachment 143455 [details] [diff] [review]
better patch

a=chofmann for 1.7b

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Fix in. 

Comment 28

[Scott MacGregor]

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

Comment 29

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

VERIFIED FIXED. I just installed eleven extensions in a row without a single
problem.

Comment 30

[Asa Dotzler [:asa]]

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