The primary bugs that caused bug 340535 to have so many dupes and comments have been fixed in bug 404609 and bug 452162. This bug is so we can start with a less noisy bug for the remaining work. Relevant remaining part of this bug from bug 340535 comment #251 At this point I believe the only file in use issues during software update would be due to a second user on the same system running the app while another user is performing a software update and when a single user is running multiple instances of the same app while performing a software update in one of the instances. This does not apply to mozMapi32.dll and MapiProxy.dll during software update which is addressed by bug 452162. What still needs to be done for this bug? Some investigation needs to be done to figure out why on the 1.8 branch jar files were in use and couldn't be copied. The reason this is of concern is that a failed partial update would create a "Frankenstein" installation that wouldn't work. This *appears* to no longer be a problem with 1.9 but it is important to be sure. Once that is done the fix should be somewhat easy to implement similar to the way it was implemented for the installer.

Please don't forget Breakpad as it is mentioned in both duplicated bugs (bug 422689 and bug 463751).

Moving over dependency list from bug 340535.

I disagree with your statement about a second user running Firefox. I've had problems with Vuze blocking the update (bug 490379). See also bug 496207

In regards to app update this bug will need to be fixed when files are in use whether it is Firefox, Vuze, or another app using the files. In regards to the installer bug 496207 will need to be fixed to workaround wither the user not being able to find the Firefox window whether it is a zombie process (common case), user didn't notice the Firefox window, or Vuze.

I experienced this problem today while testing the update process for another Gecko 1.9.2 based browser. The problem is very intermittent, occurring about 25-50% of the time I try to apply an update on one computer (WinXP) and never on a Windows 7 computer. I am very sure that no other processes on the system are running that use any of the browser files. I did some debugging and the failure occurs here: http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/update/src/updater/updater.cpp#1586. I added a GetLastError() call and it returns 32, which is ERROR_SHARING_VIOLATION. That makes me think that the browser exe has not exited yet. I added some code to dump all running processes which confirmed that the browser exe was still running. The strange thing is that the earlier call to OpenProcess() returned NULL (and it did have the correct pid). That call is here: http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/update/src/updater/updater.cpp#1422 GetLastError() in that case returns 76 which is ERROR_INVALID_PARAMETER, which I assume means the process was not found. Is it possible that the WinXP OpenProcess() call is lying? Does anyone know why the sleep() call was added here? http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/update/src/updater/updater.cpp#1431 When I have time to work on this again, I will try adding a short sleep() even when OpenProcess() returns NULL. Other tips or pointers would be greatly appreciated.

It might be that the sleep should be longer, a sleep should be added after the |if (parent) {| (looking at the code I think this would be a good thing), or that another process has it open. The comment states why it was added so I don't know what you are asking.

btw: what you are describing is actually a different bug than this one so please file a new one. Thanks!

(In reply to comment #16) > It might be that the sleep should be longer, a sleep should be added after the > |if (parent) {| (looking at the code I think this would be a good thing) as in after the |if (parent) {| block if (parent) { ... } Sleep(50);

Added a check to backup_discard so when the file doesn't exist it doesn't try to remove it.

bah, forgot to refresh

These tests don't cover everything but it is an improvement. The test patch requires the patch from bug 606410

Adds app in use and file in use tests for Windows. There are a couple of more tests I'll be adding as well.

dolske, if you don't have the time to review this I can probably get bsmedberg to.

Comment on attachment 488814 [details] [diff] [review] tests I have another test I want to add to this and will submit it tomorrow

Eek, I didn't realize quite how big a change this was. Now I feel bad for not having gotten bug 529464 in before this.

The patch is about 1/2 the size as the one in bug 529464 so it is rather minimal. It just changes things to use renames for the most part without changing major changes to the logic used. It also contains changes to the installer to handle cleanup when the user doesn't have permissions to delete on OS reboot. There are also decent tests for Windows though there isn't currently a way to test all that much on Linux / Mac OS X without mandatory locks.

forgot an include

Comment on attachment 485876 [details] [diff] [review] patch rev2 Requesting an additional review from bsmedberg in case he can find time to review this.

(In reply to comment #30) > The patch is about 1/2 the size as the one in bug 529464 so it is rather > minimal. It just changes things to use renames for the most part without > changing major changes to the logic used. Yeah, but it was mostly the file renaming stuff that I thought carried the most risk in 529464 (and I'm really being over-conservative). Since we want to do that anyway now, in for a penny in for a pound? I'd actually feel safer taking the rest of 529464 along with this. Let's talk tomorrow about what to do with that; probably makes sense to take this basically as-is, and then update the other bug to go on top/after this (the other option being to just roll this into the other bug). 529464 will be 1/2 the size now, right? :)

Comment on attachment 485876 [details] [diff] [review] patch rev2 >+ LOG(("ensure_remove: failed to remove file: " LOG_S \ >+ ", rv: %d, err: %d\n", path, rv, errno)); Nit: it's a little weird to mix compound strings and line continuations, would be neater to just linebreak at the comma. Otherwise, everything basically looks fine. I'd like to make one more nitpicky pass before r+ing, though.

(In reply to comment #33) > (In reply to comment #30) > > > The patch is about 1/2 the size as the one in bug 529464 so it is rather > > minimal. It just changes things to use renames for the most part without > > changing major changes to the logic used. > > Yeah, but it was mostly the file renaming stuff that I thought carried the most > risk in 529464 (and I'm really being over-conservative). Since we want to do > that anyway now, in for a penny in for a pound? I'd actually feel safer taking > the rest of 529464 along with this. My only fear with doing renames are the edgecases with using rename vs. copying a file using fread and fwrite. Other than that, the tests along with my manual testing using cases that aren't possible with tinderbox have shown the success and failure cases work the same with the changes as without. > Let's talk tomorrow about what to do with that; probably makes sense to take > this basically as-is, and then update the other bug to go on top/after this > (the other option being to just roll this into the other bug). 529464 will be > 1/2 the size now, right? :) Actually, it is quite doubtful that bug 529464 will be half the size. As I mentioned, this changes the minimal amount of code to accomplish renames and it also includes changes to the installer. Beyond that, it includes some unrelated logging cleanup. With all of that this patch is still around half the size as the patch in bug 529464 so I suspect the patch in bug 529464 includes many changes that are unrelated to doing renames. If I were to factor out the installer and the logging changes I suspect the patch would be around a fourth the size of the patch in bug 529464.

dolske, I've asked Dave to review the tests since he is familiar with the update tests already. Feel free to do a drive by if you have time.

btw: the tests pass on try

Comment on attachment 491150 [details] [diff] [review] patch - updated to comment >+++ b/toolkit/mozapps/update/updater/updater.cpp ... >+#ifndef WINCE >+# define DELETE_DIR L"tobedeleted" >+#endif To be is to do. — Sartre To do is to be. — Socrates Do be do be do. — Sinatra >+ if (_wrmdir(DELETE_DIR)) { >+ LOG(("NS_main: failed to remove directory: " LOG_S ", err: %d\n", >+ DELETE_DIR, errno)); Since this will be a common condition when a removed backup file was still in use, worth rephrasing to make it a bit less alarming? Ditto for LOG(("backup_discard: unable to remove ...")). OTOH, these are both be immediately followed by "so instead we..." log messages, so I suppose it's ok as is.

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/31504d265266 http://hg.mozilla.org/mozilla-central/rev/7bb86e5cc3b7

Robert, so this resolves any remaining bug we had, i.e. with attached debugger and so on?

I don't recall the debugger issue and on Windows if you restart any process that is being debugged the debugger will no longer be debugging the process which is due to the way Windows is designed since the process will get a new id and there is no way to force it to have the original id on Windows. As for all of the remaining bugs, I wouldn't be surprised if this doesn't resolve all of them though I can't be certain since all is too vague. This does resolve the issue where it is not possible to update if there is any file that needs to be updated that is in use except for firefox.exe (thunderbird.exe, seamonkey.exe, etc.). The requirement that the firefox.exe file is not in use (e.g. Firefox is currently running) has to do with if other files firefox.exe utilizes were updated and one or more of those file were not in memory the updated file could be loaded and hence cause the running instance of Firefox to appear or even be broken.

Went through the bugs that were duped and they *should* all be addressed except for bug 498483 which is the bug you filed to support updating while a debugger is attached which I just wontfix'd for the reason stated in bug 498483 comment #8. I'll likely file a new bug in the hope of finding a reasonable way to update the firefox.exe but I'm going to think about it for a while before doing so.

I have checked mostly all the different situations as given by the duplicates. Looks fine so far with Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101203 Firefox/4.0b8pre. Marking as verified fixed.

Comment on attachment 496435 [details] [diff] [review] 1.9.2 patch Approved for 1.9.2.14, a=dveditz for release-drivers

Pushed to mozilla-1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ec3643a549fc http://hg.mozilla.org/releases/mozilla-1.9.2/rev/894f9f8a4bd1

Backed out of 1.9.2 to investigate if this caused bug 633869

The tests that haven't landed for this bug are in bug 601518

Comment on attachment 517684 [details] [diff] [review] Updated 1.9.2 patch a=LegNeato for 1.9.2.16

Pushed to mozilla-1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0ef1834d18f2

I'm getting this error since 2011 now, but i've also observed a very strange behavior in Windows Explorer (Win 7 Home Premium x64), Nightly last version 13.0a1 (2012-02-26)... If i delete an executable file with shift+delete, it dissapears, after a few seconds REAPPEARS, and after a minute or so, it finally goes away... This behavior doesnt happen with the normal delete (the file goes to the Recycle Bin)... Hope this helps, maybe the update process is getting the same problem, (it attempts to delete, but the .exe won't go away?) it's very annoying (although it's clearly not a Nightly bug per se)...