Closed Bug 341161 Opened 18 years ago Closed 18 years ago

nsXPInstallManager doesnt shutdown correctly.


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

Not set


(Not tracked)



(Reporter: mossop, Assigned: mossop)



(Keywords: fixed1.8.1, memory-leak)


(2 files, 1 obsolete file)

If an nsIXPInstallManager is created and inited through xpcom then it will fail to shutdown correctly and stop observing the xpinstall-progress topic.
Blocks: 335757
Attached patch Set mNeedsShutdown in init (obsolete) — Splinter Review
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.
Assignee: xpi-engine → mossop.bugzilla
Attachment #225192 - Flags: superreview?(dveditz)
Attachment #225192 - Flags: review?(dveditz)
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.
Keywords: mlk
This is needed to fix bug 335757
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
dveditz, can you please review this ASAP? Pretty major user impact here ...
Target Milestone: --- → mozilla1.8.1beta1
Target Milestone: mozilla1.8.1beta1 → mozilla1.8.1beta2
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 ...)
Attachment #225192 - Flags: superreview?(dveditz)
Attachment #225192 - Flags: superreview+
Attachment #225192 - Flags: review?(dveditz)
Attachment #225192 - Flags: review+
Attached patch patch rev 2Splinter Review
Made the suggested change, carrying over r/sr.
Attachment #225192 - Attachment is obsolete: true
Attachment #229697 - Flags: superreview+
Attachment #229697 - Flags: review+
Whiteboard: [checkin needed]
mozilla/xpinstall/src/nsXPInstallManager.cpp 	1.146
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attached patch Branch patchSplinter Review
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.
Attachment #230168 - Flags: superreview+
Attachment #230168 - Flags: review+
Attachment #230168 - Flags: approval1.8.1?
Comment on attachment 230168 [details] [diff] [review]
Branch patch

a=darin on behalf of drivers
Attachment #230168 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [checkin needed (1.8 branch)]
Checked in to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.