466778 - [Win] Unable to update when files to be patched are in use on Windows (this is a Windows specific bug)

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving over dependency list from bug 340535.

Comment 11

[alanjstr]

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

Comment 12

[Robert Strong (they/them - no direct email)]

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.

Comment 15

[Mark Smith [:mcs]]

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.

Comment 16

[Robert Strong (they/them - no direct email)]

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.

Comment 17

[Robert Strong (they/them - no direct email)]

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

Comment 18

[Robert Strong (they/them - no direct email)]

(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);

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

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

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

bah, forgot to refresh

Comment 24

[Robert Strong (they/them - no direct email)]

Attachment: some tests

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

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: tests

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

Comment 27

[Robert Strong (they/them - no direct email)]

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

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 488814 [details] [diff] [review]
tests

I have another test I want to add to this and will submit it tomorrow

Comment 29

[Justin Dolske [:Dolske]]

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.

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: tests

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.

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: tests

forgot an include

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 485876 [details] [diff] [review]
patch rev2

Requesting an additional review from bsmedberg in case he can find time to review this.

Comment 33

[Justin Dolske [:Dolske]]

(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 34

[Justin Dolske [:Dolske]]

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.

Comment 35

[Robert Strong (they/them - no direct email)]

(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.

Comment 36

[Robert Strong (they/them - no direct email)]

Attachment: patch - updated to comment

Comment 37

[Robert Strong (they/them - no direct email)]

Attachment: tests

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.

Comment 38

[Robert Strong (they/them - no direct email)]

btw: the tests pass on try

Comment 39

[Justin Dolske [:Dolske]]

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.

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: Patch as pushed - one minor comment change

Comment 41

[Robert Strong (they/them - no direct email)]

Attachment: Test patch as pushed

Comment 42

[Robert Strong (they/them - no direct email)]

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

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 44

[Robert Strong (they/them - no direct email)]

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.

Comment 45

[Robert Strong (they/them - no direct email)]

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.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 48

[Robert Strong (they/them - no direct email)]

Attachment: 1.9.2 patch

Comment 49

[Robert Strong (they/them - no direct email)]

Attachment: 1.9.2 tests

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 496435 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.14, a=dveditz for release-drivers

Comment 51

[Robert Strong (they/them - no direct email)]

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

Comment 52

[Robert Strong (they/them - no direct email)]

Backed out of 1.9.2 to investigate if this caused bug 633869

Comment 53

[Robert Strong (they/them - no direct email)]

Attachment: Updated 1.9.2 patch

Comment 54

[Robert Strong (they/them - no direct email)]

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

Comment 55

[LegNeato]

Comment on attachment 517684 [details] [diff] [review]
Updated 1.9.2 patch

a=LegNeato for 1.9.2.16

Comment 56

[Robert Strong (they/them - no direct email)]

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

Comment 58

[Pablo Aguilar]

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)...