Last Comment Bug 286355 - Need win32 implementation of nsIProfileUnlocker
: Need win32 implementation of nsIProfileUnlocker
Status: RESOLVED FIXED
: helpwanted
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: unspecified
: x86 Windows 2000
-- normal with 14 votes (vote)
: mozilla34
Assigned To: Aaron Klotz [:aklotz]
: Florin Mezei, QA (:FlorinMezei)
: Benjamin Smedberg [:bsmedberg]
Mentors:
: 299576 301056 324516 353498 414659 452770 897402 990464 (view as bug list)
Depends on: 1068657 1112710 253950 1046942 1057868
Blocks: zombieproc start-faster 985921 1054469 1143958 982324
  Show dependency treegraph
 
Reported: 2005-03-15 20:25 PST by Benjamin Smedberg [:bsmedberg]
Modified: 2015-11-23 10:44 PST (History)
49 users (show)
mmucci: firefox‑backlog+
benjamin: blocking1.8b5-
mconnor: blocking‑aviary1.5-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 34.3
Points: 13
Has Regression Range: ---
Has STR: ---
34+


Attachments
Pseudocode (4.86 KB, text/plain)
2005-03-21 13:47 PST, Harry Johnston
no flags Details
Patch using Restart Manager (12.45 KB, patch)
2014-07-31 11:42 PDT, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Patch using Restart Manager (12.39 KB, patch)
2014-07-31 12:31 PDT, Aaron Klotz [:aklotz]
benjamin: review-
Details | Diff | Splinter Review
Patch using Restart Manager (v2) (13.56 KB, patch)
2014-08-13 18:52 PDT, Aaron Klotz [:aklotz]
benjamin: review+
Details | Diff | Splinter Review

Description User image Benjamin Smedberg [:bsmedberg] 2005-03-15 20:25:15 PST
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).
Comment 1 User image Harry Johnston 2005-03-16 12:37:15 PST
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.
Comment 2 User image Benjamin Smedberg [:bsmedberg] 2005-03-16 12:46:21 PST
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.
Comment 3 User image David Bradley 2005-03-16 12:55:43 PST
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.
Comment 4 User image Harry Johnston 2005-03-16 13:29:55 PST
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.
Comment 5 User image David Bradley 2005-03-20 18:19:31 PST
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.
Comment 6 User image Harry Johnston 2005-03-21 13:47:20 PST
Created attachment 178147 [details]
Pseudocode

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.
Comment 7 User image Benjamin Smedberg [:bsmedberg] 2005-03-21 17:06:16 PST
> 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?
Comment 8 User image Harry Johnston 2005-03-22 13:43:26 PST
I guess the executable name could be written to the lock file along with the PID?

  Harry.
Comment 9 User image Benjamin Smedberg [:bsmedberg] 2005-03-22 15:02:15 PST
sure... whatever we do, we need to do it right, because we're not going to get a
second chance.
Comment 10 User image Jaime Mitchell (use bugmail@jaimem.org.uk for email) 2005-07-03 15:27:40 PDT
*** Bug 299576 has been marked as a duplicate of this bug. ***
Comment 11 User image Benjamin Smedberg [:bsmedberg] 2005-07-18 05:54:30 PDT
*** Bug 301056 has been marked as a duplicate of this bug. ***
Comment 12 User image Darin Grimm 2005-07-18 10:15:53 PDT
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.
Comment 13 User image Benjamin Smedberg [:bsmedberg] 2005-07-18 10:21:39 PDT
What are the l10n dependencies? I'm pretty sure all the strings are already in
place.
Comment 14 User image Darin Grimm 2005-07-18 12:56:34 PDT
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?
Comment 15 User image Mike Beltzner [:beltzner, not reading bugmail] 2005-07-19 16:31:45 PDT
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.
Comment 16 User image Benjamin Smedberg [:bsmedberg] 2005-07-25 09:18:09 PDT
The bug about wording is 302039, that's the blocker.
Comment 17 User image Bradley Chapman (not reading bugmail, still gone but not forever) 2005-09-02 02:11:40 PDT
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).
Comment 18 User image Benjamin Smedberg [:bsmedberg] 2006-01-24 08:01:33 PST
*** Bug 324516 has been marked as a duplicate of this bug. ***
Comment 19 User image Mike Beltzner [:beltzner, not reading bugmail] 2006-09-20 08:02:16 PDT
*** Bug 353498 has been marked as a duplicate of this bug. ***
Comment 20 User image Worcester12345 2007-11-14 22:32:30 PST
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.
Comment 21 User image Benjamin Smedberg [:bsmedberg] 2009-02-17 05:23:27 PST
*** Bug 452770 has been marked as a duplicate of this bug. ***
Comment 22 User image Carlo Alberto Ferraris 2009-02-17 23:31:45 PST
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.
Comment 23 User image Robert Strong [:rstrong] (use needinfo to contact me) 2009-02-18 00:03:02 PST
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/
Comment 24 User image Robert Strong [:rstrong] (use needinfo to contact me) 2009-02-18 00:07:21 PST
It is likely that we wouldn't want to enable the SeDebugPrivilege for the app's token
Comment 25 User image Kevin Brosnan 2009-08-18 13:01:47 PDT
*** Bug 414659 has been marked as a duplicate of this bug. ***
Comment 26 User image Worcester12345 2009-09-10 14:23:31 PDT
Seems to be doing this a lot more in 3.5.2 version. Not sure on 3.5.3 as of yet.
Comment 27 User image Benjamin Smedberg [:bsmedberg] 2012-02-28 11:08:10 PST
http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx has information on how to do this!
Comment 28 User image Swarnava Sengupta (:Swarnava) 2013-07-28 23:24:08 PDT
*** Bug 897402 has been marked as a duplicate of this bug. ***
Comment 29 User image Benjamin Smedberg [:bsmedberg] 2014-04-07 05:58:49 PDT
*** Bug 990464 has been marked as a duplicate of this bug. ***
Comment 30 User image Aaron Klotz [:aklotz] 2014-07-31 11:42:20 PDT
Created attachment 8465661 [details] [diff] [review]
Patch using Restart Manager

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.
Comment 31 User image Aaron Klotz [:aklotz] 2014-07-31 12:31:42 PDT
Created attachment 8465692 [details] [diff] [review]
Patch using Restart Manager

Removed an extraneous comment
Comment 32 User image Benjamin Smedberg [:bsmedberg] 2014-08-07 11:43:27 PDT
Planning on getting to this on Monday.
Comment 33 User image Benjamin Smedberg [:bsmedberg] 2014-08-11 10:35:49 PDT
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.
Comment 34 User image Aaron Klotz [:aklotz] 2014-08-13 18:52:43 PDT
Created attachment 8472756 [details] [diff] [review]
Patch using Restart Manager (v2)


> 
> >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 35 User image Benjamin Smedberg [:bsmedberg] 2014-08-14 06:22:08 PDT
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!
Comment 36 User image noitidart 2014-08-14 06:56:24 PDT
Thanks all for the hard work. Just a quick question. Is there a chance of WinXP support for this?
Comment 37 User image Benjamin Smedberg [:bsmedberg] 2014-08-14 07:09:02 PDT
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).
Comment 38 User image noitidart 2014-08-14 07:11:39 PDT
Aw dang ok thanks for fast reply. Lots of users on XP though I heard wouldn't that extra data help us out?
Comment 39 User image Aaron Klotz [:aklotz] 2014-08-14 10:10:34 PDT
(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!
Comment 40 User image Benjamin Smedberg [:bsmedberg] 2014-08-14 12:08:37 PDT
ok
Comment 42 User image Ed Morley [:emorley] 2014-08-21 05:36:02 PDT
https://hg.mozilla.org/mozilla-central/rev/3ef71326b263
Comment 43 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-08-21 13:43:05 PDT
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.
Comment 44 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-08-21 16:40:15 PDT
(bug 1057052)
Comment 45 User image noitidart 2014-08-22 18:34:10 PDT
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.
Comment 46 User image Aaron Klotz [:aklotz] 2014-08-23 11:03:45 PDT
(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.
Comment 47 User image Florian Bender 2014-08-23 15:59:33 PDT
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)]:
Comment 48 User image Jacek Caban 2014-08-26 04:53:13 PDT
I fixed a trivial fixup for crosscompiling on case sensitive OSes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b290e959ff
Comment 49 User image Ryan VanderMeulen [:RyanVM] 2014-08-26 13:11:31 PDT
https://hg.mozilla.org/mozilla-central/rev/f1b290e959ff
Comment 50 User image Xu Zhen 2014-08-26 21:00:13 PDT
Why not use GetModuleFileNameEx instead of QueryFullProcessImageName for WinXP?
Comment 51 User image Aaron Klotz [:aklotz] 2014-08-27 09:15:15 PDT
(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.
Comment 52 User image Sylvestre Ledru [:sylvestre] 2014-08-28 05:01:50 PDT
Added to the release notes: "Improved "Firefox is already running" dialog with an option to recover from a locked Firefox process"
Comment 53 User image Florin Mezei, QA (:FlorinMezei) 2014-08-29 08:55:59 PDT
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>?
Comment 54 User image Jacek Caban 2014-09-02 02:11:08 PDT
I landed a trivial fixup for mingw:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3aa5e7c39e4
Comment 55 User image Aaron Klotz [:aklotz] 2014-09-02 09:45:38 PDT
(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.
Comment 56 User image Florin Mezei, QA (:FlorinMezei) 2014-09-02 09:52:55 PDT
(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?
Comment 57 User image Aaron Klotz [:aklotz] 2014-09-02 10:12:01 PDT
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?
Comment 58 User image Ryan VanderMeulen [:RyanVM] 2014-09-02 11:43:47 PDT
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.
Comment 59 User image Florin Mezei, QA (:FlorinMezei) 2014-09-05 05:56:04 PDT
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?
Comment 60 User image Aaron Klotz [:aklotz] 2014-09-05 09:09:52 PDT
(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.
Comment 61 User image noitidart 2014-09-20 02:54:55 PDT
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?
Comment 62 User image noitidart 2014-09-20 02:56:39 PDT
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?
Comment 63 User image noitidart 2014-10-08 15:25:55 PDT
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 :(
Comment 64 User image Benjamin Smedberg [:bsmedberg] 2014-10-10 13:17:26 PDT
I really don't think that the extra work to use an undocumented Windows XP feature is worth the extra engineering.
Comment 65 User image noitidart 2014-10-10 16:14:43 PDT
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
Comment 66 User image Worcester12345 2014-10-23 17:41:18 PDT
(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.
Comment 67 User image Worcester12345 2015-11-23 09:41:47 PST
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
Comment 68 User image Aaron Klotz [:aklotz] 2015-11-23 10:44:28 PST
That's probably bug 1112710.

Note You need to log in before you can comment on or make changes to this bug.