341161 - nsXPInstallManager doesnt shutdown correctly.

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

If an nsIXPInstallManager is created and inited through xpcom then it will fail to shutdown correctly and stop observing the xpinstall-progress topic.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Set mNeedsShutdown in init

When creating an instance of nsIXPInstallManager and calling either of the init methods available through xpcom, mNeedsShutdown is never set to true. This means that when the manager shutsdown it actually skips all of the shutdown in particular it doesn't attempt to stop observing the xpinstall-progress topic and so can start doing strange things when a new extension installation starts.

This patch sets mNeedsShutdown to true and also calls shutdown in the places where the initialisation fails.

Comment 2

[Dave Townsend [:mossop]]

Since the manager addrefs itself in the constructor and then doesn't release itself again unless properly shutdown I guess this is going to be a memory leak.

Comment 3

[Robert Strong (they/them - no direct email)]

This is needed to fix bug 335757

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

dveditz, can you please review this ASAP? Pretty major user impact here ...

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 225192 [details] [diff] [review]
Set mNeedsShutdown in init

>-    return Observe(aListener, XPI_PROGRESS_TOPIC, NS_LITERAL_STRING("open").get());
>+    rv = Observe(aListener, XPI_PROGRESS_TOPIC, NS_LITERAL_STRING("open").get());
>+    if (NS_FAILED(rv))
>+        Shutdown();
>+    return rv;

My only worry here is that Observe might return a failure while also calling DownloadNext() -- that might lead to crashes, too.

XPInstall is perfectly happy without a dialog to update so calling DownloadNext() even if we can't get a proxy to the dialog is OK -- but now you probably don't want that to return a failure and trigger a shutdown.

r/sr=dveditz **IFF** you change nsXPInstallManager::Observe() to not set rv on the
  rv = pmgr->GetProxyForObject(... dlg ...)

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Made the suggested change, carrying over r/sr.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpinstall/src/nsXPInstallManager.cpp 	1.146

Comment 8

[Dave Townsend [:mossop]]

Attachment: Branch patch

This is exactly the same fix ported to the branch. The differences are caused by the nsIThreadManager landing. Since this does exactly the same as the previous patch I don't believe that there is a need to re-request review.

Comment 9

[Darin Fisher]

Comment on attachment 230168 [details] [diff] [review]
Branch patch

a=darin on behalf of drivers

Comment 10

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH