422826 - crashes [@ nsVoidArray::Count] called from nsXPInstallManager::DownloadNext

Status: RESOLVED FIXED

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

Description

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

Breakpad is showing a bunch of Firefox 3.0b5pre crashes at nsVoidArray::Count called from nsXPInstallManager::DownloadNext.  These look like they're a null dereference (the void array itself is null).  In other words, mTriggers is null here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpinstall/src/nsXPInstallManager.cpp&rev=1.160&mark=800#800

(nsXPITriggerInfo::Size() is inline, and calls mItems.Count(), which is also inline.  mItems is the first object in nsXPITriggerInfo.)

Some example reports:
bp-f9a56ce6-f04d-11dc-b67c-001a4bd43ef6
bp-ba8efe47-f025-11dc-a38e-001a4bd43ed6
bp-ddc8ce0f-f003-11dc-8688-001a4bd46e84

all of which have the stack:

0  	nsVoidArray::Count()  	 nsVoidArray.h:63
1 	nsXPInstallManager::DownloadNext() 	mozilla/xpinstall/src/nsXPInstallManager.cpp:800
2 	nsXPInstallManager::Observe(nsISupports*, char const*, unsigned short const*) 	mozilla/xpinstall/src/nsXPInstallManager.cpp:572
3 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2369

Comment 1

[Dave Townsend [:mossop]]

Can't see why that would be happening at first glance. the nsXPInstallManager should be calling the EM frontend, which passes it on to the EM backend which then calls nsXPInstallManager::Observe. Right before it does that however mTriggers is dereferenced and obviously not crashing.

Comment 2

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

(In reply to comment #1)
> Right before it does that however
> mTriggers is dereferenced and obviously not crashing.

Could one of the calls between "right before it does that" and the crash call into something that icalls back into the nsXPInstallManager and sets mTriggers to null?

Comment 3

[Dave Townsend [:mossop]]

bp-8544f52e-efa3-11dc-964e-001a4bd46e84 has a potentially useful comment "Dropped three .xpi extensions from desktop to try install several in sequence", still not able to reproduce though.

Comment 4

[Damon Sicore (:damons)]

+'ing w/ P2.  Let's get an owner for this so we can investigate.  Mossop?  dbaron?

Comment 5

[Dave Townsend [:mossop]]

I can take some time to try to figure out what might be going on.

Comment 6

[Damon Sicore (:damons)]

Thanks, Mossop.  Really appreciate the help.

Comment 7

[Dave Townsend [:mossop]]

Before PGO this had a different top stack, so this is happening on all platforms going back to 2007091216 as nsXPInstallManager::DownloadNext().

This stacks are better on other platforms and before PGO too, this is the best one I've found:

http://crash-stats.mozilla.com/report/index/10d53bae-f50f-11dc-8c08-001a4bd43ed6

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Finally figured out how to reproduce this but it isn't in any way that you would normally do in a clean install, so I suspect an extension is involved. What you need to do is open two windows of the extension manager.

The notification of a new install from nsXPInstallManager to the progress dialog (the extension manager window) is not ideal. When the dialog is already open it simply notifies through the observer service. If there are two windows open then this message gets to both of them. The first that sees it will start the install. In the event that the install is from an xpi file on the local file system the install will complete immediately so that when the second window receives the notification it attempts to start the install after it has already completed, hence the trigger list has already been destroyed.

The suggested fix is pretty simple. mDialogOpen is used to avoid attempting to restart installs that have already been started, but right now after all installs are complete it is reset. This allows the second dialog to start them again. Just not resetting it fixes the crash, the nsXPInstallManager instance is not meant to be reusable to my knowledge so I think this is the right fix.

All code in the tree is protected against opening a second copy of the extension manager however it is possible to put "chrome://mozapps/content/extensions/extensions.xul" into the address bar and then open the extension manager in the normal way to get two instances. Then just drag an xpi file into a different tab in the browser and it will start the install and crash.

Comment 9

[Dave Townsend [:mossop]]

This patch doesn't totally remove any problems caused by having two extension manager windows open, it leaves some UI oddities in place of the crash, but I can see no way to cause this without an extension installed and a fuller fix would be somewhat more complicated, so I think anything additional should not block 1.9, though we may want to do something about it later.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 313661 [details] [diff] [review]
patch rev 1

r=dveditz

Comment 11

[Dave Townsend [:mossop]]

Checked in, will keep an eye on the crash stats to make sure this has solved it.

Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.164; previous revision: 1.163
done