As stated in bug 1375242 comment #7 when the Firefox process doesn't exit and we try to perform an update trying to relaunch Firefox is not successful. Though this won't fix the root of the problem which is Firefox not exiting it might be a good thing to not try relaunching Firefox when this happens.

The update should still be applied otherwise the client won't be able to update to a version that contains fixes to Firefox that are preventing it from exiting.

I'll make sure this passes try before landing. I didn't add tests for the pid successfully exiting since those are already handled by the AppApply* tests.

Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdfad3e4b3dce3930d9d9538a0b139d151925550

While working on this I considered adding whether the process didn't exit per the updater to telemetry but there isn't a decent way to do this at this time. I'll likely add it in a followup bug.

Comment on attachment 8880962 [details] [diff] [review] patch rev1 Review of attachment 8880962 [details] [diff] [review]: ----------------------------------------------------------------- Simple enough change, can't think of anything it would break that wouldn't already have to be broken, and there's a test added, I'm happy. ::: toolkit/mozapps/update/updater/updater.cpp @@ +2119,5 @@ > + // pid is still present. > + if (pid > 0) { > + HANDLE parent = OpenProcess(SYNCHRONIZE, false, (DWORD) pid); > + if (parent) { > + return; Wouldn't hurt to close the handle. @@ +3024,5 @@ > if (result != WAIT_OBJECT_0) { > + // Continue to update since the callback application sometimes doesn't > + // exit (see bug 1375242) so any fixes to the callback application will > + // be applied instead of leaving the client in a broken state. > + LOG(("The callback process didn't exit! Continuing with update.")); It's really the parent process we're waiting for here, not the callback. It happens that those are the same binary (though I don't think we explicitly require or even say that anywhere), but they are distinct processes, so talking about the callback here could get confusing.

(In reply to Matt Howell [:mhowell] from comment #5) > Comment on attachment 8880962 [details] [diff] [review] > patch rev1 > > Review of attachment 8880962 [details] [diff] [review]: > ----------------------------------------------------------------- > > Simple enough change, can't think of anything it would break that wouldn't > already have to be broken, and there's a test added, I'm happy. > > ::: toolkit/mozapps/update/updater/updater.cpp > @@ +2119,5 @@ > > + // pid is still present. > > + if (pid > 0) { > > + HANDLE parent = OpenProcess(SYNCHRONIZE, false, (DWORD) pid); > > + if (parent) { > > + return; > > Wouldn't hurt to close the handle. doh! > > @@ +3024,5 @@ > > if (result != WAIT_OBJECT_0) { > > + // Continue to update since the callback application sometimes doesn't > > + // exit (see bug 1375242) so any fixes to the callback application will > > + // be applied instead of leaving the client in a broken state. > > + LOG(("The callback process didn't exit! Continuing with update.")); > > It's really the parent process we're waiting for here, not the callback. It > happens that those are the same binary (though I don't think we explicitly > require or even say that anywhere), but they are distinct processes, so > talking about the callback here could get confusing. Thanks for the review and will do.

carrying forward r+

Pushed by rstrong@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf241f97c71 When the Firefox process doesn't exit apply the update anyway and don't relaunch the callback application. r=mhowell

I don't know whether it was applied in today update, but when I clicked "Restart...", I went to Task Manager - there were two Firefox processes, one exited, second remained and I had to kill this so update could proceed.

It wasn't and this should merge into Nightly in a couple of days. It will be in the build the day after this bug is marked fixed. This bug also has no affect on the firefox process not exiting which is outside of app update's control though app update will hopefully not launch an additional firefox process when firefox is experiencing the problem where it doesn't exitproperly. The firefox process not exiting will be handled in bug 1375242.

For those following along, this patch has now landed and will be in tomorrow's build. So, updating when already using tomorrow's build or a build after tomorrow's build will have this new behavior.

It will be hard to tell whether it works like it should, because in today build all processes exit after closing Nightly ;).

Today processes didn't exit and update still couldn't proceed.

It might be that firefox startup didn't run due to the process not restarting. Instead of restarting just exit then make sure there are no firefox.exe processes and then launch Firefox. Whatever the case, this is about as much as can be done from the updater side for the firefox.exe process not exiting.

But as always, I killed firefox processes and update proceed.

Right and understood. That is the firefox.exe process not exiting. That should be discussed in the bug regarding that.

I don't know where to write, here or here: https://bugzilla.mozilla.org/show_bug.cgi?id=1376612 but still Nightly can't normally update. It is not fixed!

This is a best effort to update anyways even without bug 1375242 fixed. I am not at all surprised that update anyways for everyone and bug 1375242 will need to be fixed for the root cause preventing you from updating to be fixed. As shown by bug 1375242 comment#27 this did make it so that client was able to update even without bug 1375242 fixed. So yes, this bug is fixed!

I don't know, but I see there: "Window opened updating files Window closed nightly did not start 2 processes open Started nightly 3rd opened nightly did not start Ended processes Started nightly opened normally" - so just like me Bob had to kill processes so that update could proceed.

It seems to be after update process... We will see tomorrow ;).

The issues regarding having to kill the process is bug 1375242 and has nothing to do with this bug. In the case of Bob it updated without killing the process and after he killed the process he was able to launch. Once again, this bug has nothing to do with the Firefox process having to be killed, etc.

Then I don't understand the aim of this bug 1375549 if still user can not use Nightly unless he manually kills processes, but ok, I'm not browsers specialist and it's not important.

It is so the update process proceeds when it can so when someone experiences bug 1375242 that at least they get updated to the latest version available. This way when someone fixes bug 1375242 they will be updated to a version that contains the fix for bug 1375242.

See Bug 1374043 "Systematic Nightly 56 crash on exit (+ update hang)" I firmly believe that bug 1374043 is contributing to the 'Nightly updating issues', seen since 2017-06-15, on Windows Nightly 64 bit builds. That bug is 'long and convoluted'. Part of the 'issue' is that there are no 'Firefox crash reports' JUST a 'Windows Crash' (when you close Nightly). Possible duplicates of that bug include: Bug 1373918 "Nightly is crashing at close" and Bug 1376762 "Firefox Nightly (56) 20170615+ crashes on exit (Windows 7 & 10 64bit)" The latter, is a concise report - and has a Regression pointing to Bug 1372375 DJ-Leith

Quite possible but commenting in this bug regarding the Firefox process having problems isn't going to do anyone any good. Bug 1375242 is the bug you should be commenting on.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #24) > It is so the update process proceeds when it can so when someone experiences > bug 1375242 that at least they get updated to the latest version available. > This way when someone fixes bug 1375242 they will be updated to a version > that contains the fix for bug 1375242. But to launch Nightly to update to the version which fixes bug 1375242, user still has to kill processes? This is what I do every time for last days to update. If I don't kill processes, the update won't proceed. If Bob doesn't kill processes, Nightly won't launch. I see no practical difference. Nevermind, I won't understand that.

(In reply to tkdubiel from comment #27) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #24) > > It is so the update process proceeds when it can so when someone experiences > > bug 1375242 that at least they get updated to the latest version available. > > This way when someone fixes bug 1375242 they will be updated to a version > > that contains the fix for bug 1375242. > > But to launch Nightly to update to the version which fixes bug 1375242, user > still has to kill processes? This is what I do every time for last days to > update. > If I don't kill processes, the update won't proceed. If Bob doesn't kill > processes, Nightly won't launch. I see no practical difference. > Nevermind, I won't understand that. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22) > The issues regarding having to kill the process is bug 1375242 and has > nothing to do with this bug. In the case of Bob it updated without killing > the process and after he killed the process he was able to launch. Once > again, this bug has nothing to do with the Firefox process having to be > killed, etc.

:-)

Today Nightly started updating about 30 seconds after clicking "Restart...". Another 20 seconds was taken by Nightly to install updates. After all it updated (with difficulties, but as said earlier, no possibility to launch Nightly unless you kill all its processes).