Sometimes (for reasons not entirely apparent), Firefox will mostly shutdown (stop accepting DDE requests) but won't shut down completely and release its profile lock. This seems to have something to do with having loaded Java and popup blocking at the same time, or some similarly irreproducible series of events. We want to give users the option to forcefully terminate this zombie process. The code needs to figure out which current process owns the profile lock, and kill that process. The code needs to be hooked up here: http://lxr.mozilla.org/seamonkey/source/profile/dirserviceprovider/src/nsProfileLock.cpp#499 dbradley says he's willing to look at this over the weekend. Enterprising experts in the windows API are also welcome to take this bug. Something like the code in "forcedel" utility may be useful, except that we want to kill the process, not merely release the file lock: http://www.codeguru.com/Cpp/W-P/files/fileio/article.php/c1287 It's fine if this code only works in newer windows (win2k+), we can return a null nsIProfileUnlocker object on older OSes. This code, however, cannot be copied directly because it is not tri-licensed. We *can* copy header information from the mingw w32api (specifically ntapi.h).

The forcedel utility uses undocumented/internal Windows functions, which would be really bad practice for a production application like Firefox. We can achieve almost the same effect legitimately in a number of ways: A) Once we've successfully locked a profile, create an event object and an extra thread to act as a monitor. The event object name should be derived from the profile lock filename. The thread waits for the event object to be signalled then calls ExitProcess. To end the process just signal the event object. This might not work if the process is in a really peculiar state, but should be effective under most circumstances. B) Once we've successfully locked a profile, record our process ID in it somewhere (or record the process ID in the lockfile). Create an event object whose name is derived from the profile lock filename combined with the process ID. To kill the process, we read the process ID number, verify that the event object exists, then call TerminateProcess. Both of these approaches should work as far back as Windows 95. I haven't thought about it very long so there may well be superior methods. Remember though that the process might not be on the local computer, so it is important to verify that you've got the right process before terminating it! Harry.

Harry, I think that solution (B) might be effective (recoding the COMPUTERNAME:PID in the lockfile). If I'm reading the current code correctly, that approach is backwards-compatible (e.g. If we launch a Firefox 1.0 while 1.1 is running, 1.0 must still recognize that the profile is locked). Do you have the time to try implementing this solution? I can provide assistance as necessary (as can most of irc.mozilla.org#developers). I'm not sure we care all that much about profile locks from processes that aren't on the local computer, but perhaps I could be convinced to care.

The problem with both of these approaches is they are good for the future, but won't work if you want this to work for the existing Firefox 1.0x builds. But I agree with the previous comment that we should not use undocumented Win32 api functions. I haven't looked yet, so I'm not yet ready to give up on the idea externally terminating Firefox.

Benjamin: sorry, no. I'm expert in certain narrow fields, but something like Firefox is well outside my experience - and I can't afford the amount of time it would take to get up to speed. If it would help I'm happy to write some code fragments but someone else would have to fit them into context. David: yes, I'm aware this doesn't give us backwards compatibility, but I'm not sure whether that's a serious problem under normal circumstances? After all, the process will go away by itself as soon as the machine is rebooted. Regarding remote instances of Firefox, there isn't much to be done about them, we just need to be absolutely sure that we don't (a) kill a random local process when we think we're killing firefox or (b) think it's safe to ignore or override the profile lock when it isn't. It would be nice to tell the user the name of the computer the process is running on, though. Harry.

Can we count on a well known process name to check when we lookup the process by PID? What I'm thinking is that the code would read in the PID, lookup the process and verify the name, and then terminate. That would help guard against accidentally getting the wrong process, though I think Windows doesn't reuse PID's right away. Also should I furnish a patch for the lock file change to write the PID? Lastly any preference on how to store the PID in the lock file? I'm assuming I can just take the first number I find in the file.

Might be useful for David to compare with what he's got. The event object in my code is probably redundant if we check the executable module name.

> Can we count on a well known process name to check when we lookup the process by > PID? No. This is xulrunner code, so the process name might be xulrunner.exe and you don't know which XUL application a particular xulrunner.exe process is running. > Also should I furnish a patch for the lock file change to write the PID? Yes. > Lastly any preference on how to store the PID in the lock file? I'm assuming I > can just take the first number I find in the file. How about MACHINENAME:PID:CreationDate or somesuch?

I guess the executable name could be written to the lock file along with the PID? Harry.

sure... whatever we do, we need to do it right, because we're not going to get a second chance.

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

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

Depending on the resolution of this bug, there may be 110n dependancies so it really needs to go in before the freeze. This bug fix is needed to finally cleanup the "profile in use" issue for average users.

What are the l10n dependencies? I'm pretty sure all the strings are already in place.

There probably are no 110n dependancy issues if this bug gets fixed as proposed. However, if this bug does not get fixed for 1.8/Firefox 1.1, the bug I filed that was duped to this one was for a simple text string to be added to the dialog indicating a machine restart may be required. I assume that would be a new string. Beltzner, do you have any comment on this from a usability perspective? Is this as big a support issue for average users as I suspect?

Well, it certainly seems to come up a lot (as indicated in bug 253950, comment 8, and from my own anecdotal experience hanging around in #firefox), so I think that it's safe to say that it's possible and even probabe that this case will be encountered by the average user. The current dialog (see attachment 189566 [details]) is an improvement over 1.0.x, but will still strand the majority of our users who won't understand why if "Firefox is running" they don't see it on the taskbar. Even of those who are smart enough to know what the task manager is, it's a subset who know how to kill a lurking process. Ideally we'd get a fix in that allows us to simply kill the errant processes (the installer and update system seem to be able to do this) and then we could show a dialog like the one bsmedberg mentions in bug 253950, comment 25. If that isn't possible in the 1.1 timeframe, then I think a slight modification of the current dialog will go a long way to helping users, even though getting them to restart the system sucks: Firefox is already running, but not responding. To open a new window, you must first close all Firefox processes, or restart your system.

The bug about wording is 302039, that's the blocker.

Where would a good place be to begin learning about writing nsIimpl.cpp code? I'd like to do some research on such a subject, and possibly try my hand at implementing nsIProfileUnlocker on Linux (in a separate bug, if need be).

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

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

I've been seeing a lot more of this lately in both nightly branch Firefox builds and nightly trunk Seamonkey builds. So, it seems to go across the lines on both product and trunk/branch. Hope that helps.

bsmedberg asked in the last dup a way to enumerate the process(es) locking a file: always on sysinternals I found some kind of solution, even if it uses ZwQuerySystemInformation, that isn't a "frozen" API: http://forum.sysinternals.com/forum_posts.asp?TID=17533&PID=87788#87788 The example is coded in C#, so it's merely an hint.

As long as the process is not running as a different user this can be done fairly easily. Here is another example that doesn't use ZwQuerySystemInformation http://www.codeguru.com/cpp/w-p/system/processesmodules/article.php/c2827/

It is likely that we wouldn't want to enable the SeDebugPrivilege for the app's token

Seems to be doing this a lot more in 3.5.2 version. Not sure on 3.5.3 as of yet.

http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx has information on how to do this!

This patch uses the Windows Restart Manager to find the process holding the existing lock. It uses runtime dynamic linking to ensure that we don't break Windows XP support.

Removed an extraneous comment

Planning on getting to this on Monday.

Comment on attachment 8465692 [details] [diff] [review] Patch using Restart Manager >diff --git a/profile/dirserviceprovider/src/ProfileUnlockerWin.cpp b/profile/dirserviceprovider/src/ProfileUnlockerWin.cpp >+class ScopedRestartManagerSession This class needs a little bit of docs. I'm pretty sure from the name that I know what it does, but inputs/outputs should be clearer: it does its work on construction, right? What state changes does it make to its parent ProfileUnlockerWin? It should probably also be MOZ_STACK_CLASS. >+{ >+public: >+ explicit ScopedRestartManagerSession(ProfileUnlockerWin& aUnlocker) >+ : mError(ERROR_INVALID_HANDLE) >+ , mHandle((DWORD)-1) // 0 is a valid restart manager handle Use INVALID_HANDLE_VALUE ? >+ operator bool() >+ { >+ return mError == ERROR_SUCCESS; >+ } >+ >+ operator DWORD() >+ { >+ return mHandle; >+ } Having these two operators looks fragile to me. Can we make these methods like bool ok() and HANDLE handle() ? >+ >+private: >+ DWORD mError; >+ DWORD mHandle; Not HANDLE? >+nsresult >+ProfileUnlockerWin::Init() >+{ >+ if (mRestartMgrModule) { This looks assertable... we shouldn't ever be dual-initing right? >+NS_IMETHODIMP >+ProfileUnlockerWin::Unlock(uint32_t aSeverity) >+{ >+ if (!mRestartMgrModule) { >+ nsresult rv = Init(); >+ if (NS_FAILED(rv)) { >+ return rv; >+ } >+ } Why did you decide that it's better to implicit-initialize instead of explicitly initializing once when we create the object (before we pass it around)? It seems that we might end up showing unlocking UI even when we can't successfully unlock. >+ // Since we know the use case for the list, we are going to assume that there >+ // is one process accessing the lock file. >+ UINT numEntries = 1; >+ UINT numEntriesNeeded = 0; >+ RM_PROCESS_INFO info = {0}; >+ DWORD reason = RmRebootReasonNone; >+ error = mRmGetList(session, &numEntriesNeeded, &numEntries, &info, &reason); >+ if (error != ERROR_SUCCESS) { >+ MOZ_ASSERT(error != ERROR_MORE_DATA); I don't understand this assert. We assume above that there is only one process locking the file, which is usually correct, but I don't know why we'd need to assert it that way. If there is more than one process locking the file, why don't we at least kill the first one? >+ DWORD accessRights = PROCESS_QUERY_LIMITED_INFORMATION; >+ if (aSeverity == FORCE_QUIT) { >+ accessRights |= PROCESS_TERMINATE; >+ } Why this "if"? Don't we already know the severity from above? >+ WCHAR imageName[MAX_PATH]; >+ DWORD imageNameLen = MAX_PATH; >+ if (!mQueryFullProcessImageName(otherProcess, 0, imageName, &imageNameLen)) { >+ return NS_ERROR_FAILURE; >+ } >+ nsCOMPtr<nsIFile> otherProcessImageName; >+ if (NS_FAILED(NS_NewLocalFile(nsDependentString(imageName, imageNameLen), >+ false, getter_AddRefs(otherProcessImageName)))) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ imageNameLen = MAX_PATH; >+ if (!mQueryFullProcessImageName(::GetCurrentProcess(), 0, imageName, >+ &imageNameLen)) { >+ return NS_ERROR_FAILURE; >+ } >+ nsCOMPtr<nsIFile> thisProcessImageName; >+ if (NS_FAILED(NS_NewLocalFile(nsDependentString(imageName, imageNameLen), >+ false, getter_AddRefs(thisProcessImageName)))) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ // Make sure the image file names match Why? Especially if you're using nightly and beta or win32 and win64 builds, this check sounds overly conservative. I could probably support checking that the leafname matches, though... >diff --git a/profile/dirserviceprovider/src/nsProfileLock.cpp b/profile/dirserviceprovider/src/nsProfileLock.cpp >--- a/profile/dirserviceprovider/src/nsProfileLock.cpp >+++ b/profile/dirserviceprovider/src/nsProfileLock.cpp >@@ -2,16 +2,20 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsProfileStringTypes.h" > #include "nsProfileLock.h" > #include "nsCOMPtr.h" > >+#if defined(XP_WIN) >+#include "mozilla/ProfileUnlockerWin.h" >+#endif >+ > #if defined(XP_MACOSX) > #include <Carbon/Carbon.h> > #include <CoreFoundation/CoreFoundation.h> > #endif > > #ifdef XP_UNIX > #include <unistd.h> > #include <fcntl.h> >@@ -570,17 +574,21 @@ nsresult nsProfileLock::Lock(nsIFile* aP > mLockFileHandle = CreateFileW(filePath.get(), > GENERIC_READ | GENERIC_WRITE, > 0, // no sharing - of course > nullptr, > CREATE_ALWAYS, > 0, > nullptr); > if (mLockFileHandle == INVALID_HANDLE_VALUE) { >- // XXXbsmedberg: provide a profile-unlocker here! >+ if (aUnlocker) { >+ nsCOMPtr<nsIProfileUnlocker> unlocker( >+ new mozilla::ProfileUnlockerWin(filePath)); As noted above, this should be explicitly-initialized here and reset to null if we're on an old OS where this won't work. >diff --git a/toolkit/profile/moz.build b/toolkit/profile/moz.build >--- a/toolkit/profile/moz.build >+++ b/toolkit/profile/moz.build >@@ -10,16 +10,17 @@ XPIDL_SOURCES += [ > 'nsIProfileMigrator.idl', > 'nsIToolkitProfile.idl', > 'nsIToolkitProfileService.idl', > ] > > XPIDL_MODULE = 'toolkitprofile' > > UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/src/nsProfileLock.cpp' ] >+UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/src/ProfileUnlockerWin.cpp' ] Pretty sure this needs a windows check.

> > >diff --git a/toolkit/profile/moz.build b/toolkit/profile/moz.build > >--- a/toolkit/profile/moz.build > >+++ b/toolkit/profile/moz.build > >@@ -10,16 +10,17 @@ XPIDL_SOURCES += [ > > 'nsIProfileMigrator.idl', > > 'nsIToolkitProfile.idl', > > 'nsIToolkitProfileService.idl', > > ] > > > > XPIDL_MODULE = 'toolkitprofile' > > > > UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/src/nsProfileLock.cpp' ] > >+UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/src/ProfileUnlockerWin.cpp' ] > > Pretty sure this needs a windows check. Fixed.

Comment on attachment 8472756 [details] [diff] [review] Patch using Restart Manager (v2) Still need INVALID_HANDLE_VALUE in the ScopedRestartManagerSession constructor and s/DWORD/HANDLE/ for the mHandle declaration? r+ with those changes. Please file a followup bug to use something other than TerminateProcess to kill the other process: we'd probably like to get crash report stacks from this case and that involves either doing some further kind of magic. Thank you for fixing this!

Thanks all for the hard work. Just a quick question. Is there a chance of WinXP support for this?

No. WinXP doesn't expose the API necessary to do this (at least without hacking at the kernel data structures, which is not something we're prepared to do).

Aw dang ok thanks for fast reply. Lots of users on XP though I heard wouldn't that extra data help us out?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #35) > Comment on attachment 8472756 [details] [diff] [review] > Patch using Restart Manager (v2) > > Still need INVALID_HANDLE_VALUE in the ScopedRestartManagerSession > constructor and s/DWORD/HANDLE/ for the mHandle declaration? The restart manager APIs have their "handles" typed as DWORDs, not HANDLEs. > > Please file a followup bug to use something other than TerminateProcess to > kill the other process: we'd probably like to get crash report stacks from > this case and that involves either doing some further kind of magic. > Will do. > Thank you for fixing this!

ok

Comment on attachment 8472756 [details] [diff] [review] Patch using Restart Manager (v2) Excellent work, I'm glad we got this fixed! >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp > rv = ps->ConfirmEx(nullptr, killTitle, killMessage, flags, >- killTitle, nullptr, nullptr, nullptr, >+ nullptr, killTitle, nullptr, nullptr, Re-purposing this title string for the button is not ideal from an l10n point of view (using the same string in different context is generally bad, even if they are the same in English). More importantly, in this case the resulting UX is also far from ideal - you need to hit "Close Firefox" to "continue starting" Firefox, from the user's point of view. Verdi called this out on IRC and is filing a UX bug to improve this dialog, so we'll fix that there.

(bug 1057052)

Is this the dialog we see when process is still open: http://cdn.ghacks.net/wp-content/uploads/2014/08/firefox-already-running.png This dialog shows when users launches firefox while it is still running. So the "Close Firefox" button would make more sense as "Close & Restart Firefox". Otherwise users have to click "Close Firefox" then go back and click on the icon agian to launch firefox.

(In reply to noitidart from comment #45) > Is this the dialog we see when process is still open: > > http://cdn.ghacks.net/wp-content/uploads/2014/08/firefox-already-running.png > > > This dialog shows when users launches firefox while it is still running. So > the "Close Firefox" button would make more sense as "Close & Restart > Firefox". Otherwise users have to click "Close Firefox" then go back and > click on the icon agian to launch firefox. Please see bug 1057052.

Release Note Request (optional, but appreciated) [Why is this notable]: improved UX [Suggested wording]: Improved the "Firefox is already running" dialog with an option to recover from a locked Firefox process. [Links (documentation, blog post, etc)]:

I fixed a trivial fixup for crosscompiling on case sensitive OSes: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b290e959ff

Why not use GetModuleFileNameEx instead of QueryFullProcessImageName for WinXP?

(In reply to Xu Zhen from comment #50) > Why not use GetModuleFileNameEx instead of QueryFullProcessImageName for > WinXP? Using GetModuleFileNameEx would not make this patch compatible with XP. There is no documented equivalent to the restart manager APIs in Windows XP.

Added to the release notes: "Improved "Firefox is already running" dialog with an option to recover from a locked Firefox process"

I've verified this using the latest Firefox 34 Nightly (BuildID=20140829030204) on Win XP x86, Win 7 x64, Win 8 x64 and Win 8.1 x64. To get this message, I've set Firefox to not ask to pick a profile on startup, started Firefox and loaded multiple tabs (~10 tabs), then closed and quickly reopened Firefox. I was able to get the message on each of the systems I've tried this. Win XP displays the message "Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system." with <OK> button. According to comment 37 this is expected though. Win 7, 8 and 8.1 display message "Firefox is already running, but is not responding. The old Firefox process must be closed to open a new window." with <Cancel> and <Close Firefox> buttons. I cannot say with full certainty that the <Close Firefox> button works as expected as I do not know a reliable way to lock the process for a long period of time. However, I tried this multiple times, and sometimes Nightly was restarted right after pressing the button, while other times it took several seconds to be able to manually start it, but in all cases the process was closed and Nightly could be started. Aaron, do you think that the fact that sometimes it takes several seconds for the process to be killed is an issue? Should it be terminated immediately after clicking on <Close Firefox>?

I landed a trivial fixup for mingw: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3aa5e7c39e4

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #53) > Aaron, do you think that the fact that sometimes it takes several seconds > for the process to be killed is an issue? Should it be terminated > immediately after clicking on <Close Firefox>? The process is terminated aggressively; I would think that it should be fairly quick. But how long it takes is not really the issue: the important part is that clicking <Close Firefox> should always result in the browser bring restarted.

(In reply to Aaron Klotz [:aklotz] from comment #55) > The process is terminated aggressively; I would think that it should be > fairly quick. But how long it takes is not really the issue: the important > part is that clicking <Close Firefox> should always result in the browser > bring restarted. I would have to double check this but I'm pretty sure that clicking <Close Firefox> does not always restart the browser. Several times, when clicking it, the window was dismissed but Firefox was NOT reopened (I had to manually reopen it)... this happened whenever it took like 3-4 or more seconds to kill the process. Firefox was restarted after clicking the button only when it took around 1-2 seconds max for the process to be killed. Aaron, what's your take on this?

I did check in some follow up fixes to address some possible situations where this could occur in bug 1057466. Can you please ensure that your test build includes those changes?

https://hg.mozilla.org/mozilla-central/rev/f3aa5e7c39e4 Note that this landed on m-c post-uplift, i.e. Gecko 35. In general, this is why we prefer new bugs for follow-up patches.

I did some additional testing today on Win 7 x64 with the latest Firefox 34 Aurora (BuildID=20140904004005) and latest Firefox 35 Nightly (BuildID=20140904030202). I can confirm that I no longer see the behavior where clicking <Close Firefox> does not always restart the browser. I tried several times and Firefox is always restarted automatically after clicking <Close Firefox>, even if you wait longer before clicking the button. There are though two cases where Firefox does NOT start automatically. See them detailed below. The scenario I used for testing was the following: 1. Opened Firefox with a clean profile and set it to start without asking to pick a profile on startup. 2. Opened 25 tabs with different websites. 3. Closed Firefox. 4. Reopened Firefox and restored session. 5. Quickly selected all tabs so they start to load. 6. Quickly closed Firefox and attempted to restart it. Many times I got the "Firefox already running" dialog and choosing to close Firefox restarted the application as expected. However, sometimes I got the following two exceptions where Firefox was not restarted: 1. "Profile Missing" dialog after clicking <Close Firefox> - http://www.screencast.com/t/d8cXT4J3AMKZ - after dismissing the dialog, Firefox does not restart automatically - after the Firefox process is killed (automatically), then you can manually start Firefox again without getting the dialog 2. Nothing happens when trying to restart Firefox - http://www.screencast.com/t/L0pb2JceY3bP - sometimes, instead of displaying the "Firefox already running" dialog a new Firefox process is created, and nothing visible happens - multiple new processes can be launched without displaying anything - after the initial process is killed (automatically), then you can manually start Firefox again Aaron, can you review the two exceptions above and let me know: - are they issues? - if yes, should I file them separately or reopen this? - if these will be tracked separately should I close this issue?

(In reply to Florin Mezei, QA (:FlorinMezei) [on vacation - Sep 8-15 ] from comment #59) > Aaron, can you review the two exceptions above and let me know: > - are they issues? Yes, they are both issues since they affect the user experience when using the profile unlocker. > - if yes, should I file them separately or reopen this? Let's file each of them separately with their own bugs. > - if these will be tracked separately should I close this issue? I think we should make those bugs dependencies of this bug. Let's not flag this one as VERIFIED until those other two are resolved.

hey guys, instead of using restart manager, why not this way, so you can even support xp: on start of firefox profile, create a file with the pid of firefox. or registry entry. on succesfull close of firefox profile it should delete that file. then on next startup look for that file and if its still there then read the pid from the file, and prompt if you want to kill that pid?

well i guess this method wont work if say the user ctrl alt delled the firefox. or computer shutdown. but you can work around that. you can read hte pid from the file and test if its running. OR you can put a lock on that file while firefox is running and when firefox dies then its unlockd and you can access. what u guys think?

hey all i figured out the undocumented way to do it. it works on winxp. did it in jsctypes here: https://gist.github.com/Noitidart/d752e2c59793fa2cab3c can we implement this so we can support winXP? i know we dont want to use undocumented stuff but this is getting documented over at msdn now. and also its better than no support :(

I really don't think that the extra work to use an undocumented Windows XP feature is worth the extra engineering.

Do you think they'll get rid of it one day is that why? A lot of stuff seems to rely on NtQueryInformationFile and NtQuerySystemInformation

(In reply to noitidart from comment #65) > Do you think they'll get rid of it one day is that why? A lot of stuff seems > to rely on NtQueryInformationFile and NtQuerySystemInformation Microsoft isn't supporting XP any more. Why should Firefox? Time to move on.

I just wanted to report that I'm still seeing this dead background process once in a while with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 ID:20151029151421

That's probably bug 1112710.