Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Application Update
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b8
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 .17+, status1.9.2 .17-fixed)

Details

Attachments

(7 attachments, 8 obsolete attachments)

17.40 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
183.19 KB, patch
mossop
: review+
Details | Diff | Splinter Review
17.40 KB, patch
Details | Diff | Splinter Review
187.35 KB, patch
Details | Diff | Splinter Review
14.88 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
103.23 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
15.10 KB, patch
rstrong
: review+
christian
: approval1.9.2.17+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 340535
Duplicate of this bug: 422689
Duplicate of this bug: 463751
Summary: Unable to update when files to be patched are in use → [Win] Unable to update when files to be patched are in use on Windows (this is a Windows specific bug)
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.
Blocks: 314148, 394021
Flags: wanted1.9.0.x+
Duplicate of this bug: 398970
Duplicate of this bug: 490379
Duplicate of this bug: 498483
Duplicate of this bug: 461419
Duplicate of this bug: 508848

Comment 11

8 years ago
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.

Updated

8 years ago
Duplicate of this bug: 473073
No longer blocks: 314148
Blocks: 493028
Duplicate of this bug: 415013
Depends on: 529464
No longer blocks: 394021

Comment 15

7 years ago
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);
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Created attachment 485578 [details] [diff] [review]
patch in progress
Created attachment 485667 [details] [diff] [review]
patch rev1
Attachment #485578 - Attachment is obsolete: true
Attachment #485667 - Flags: review?(dolske)
Duplicate of this bug: 498611
Depends on: 525390
Created attachment 485871 [details] [diff] [review]
patch rev2

Added a check to backup_discard so when the file doesn't exist it doesn't try to remove it.
Attachment #485667 - Attachment is obsolete: true
Attachment #485871 - Flags: review?(dolske)
Attachment #485667 - Flags: review?(dolske)
Created attachment 485876 [details] [diff] [review]
patch rev2

bah, forgot to refresh
Attachment #485871 - Attachment is obsolete: true
Attachment #485876 - Flags: review?(dolske)
Attachment #485871 - Flags: review?(dolske)
Created attachment 485891 [details] [diff] [review]
some tests

These tests don't cover everything but it is an improvement. The test patch requires the patch from bug 606410
Attachment #485891 - Flags: review?(dolske)
blocking2.0: --- → betaN+
Duplicate of this bug: 609297
Created attachment 488814 [details] [diff] [review]
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.
Attachment #485891 - Attachment is obsolete: true
Attachment #488814 - Flags: review?(dolske)
Attachment #485891 - Flags: review?(dolske)
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
Attachment #488814 - Flags: review?(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.
Created attachment 489765 [details] [diff] [review]
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.
Attachment #488814 - Attachment is obsolete: true
Attachment #489765 - Flags: review?(dolske)
Created attachment 489769 [details] [diff] [review]
tests

forgot an include
Attachment #489765 - Attachment is obsolete: true
Attachment #489769 - Flags: review?(dolske)
Attachment #489765 - Flags: review?(dolske)
Comment on attachment 485876 [details] [diff] [review]
patch rev2

Requesting an additional review from bsmedberg in case he can find time to review this.
Attachment #485876 - Flags: review?(benjamin)
(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.
Created attachment 491150 [details] [diff] [review]
patch - updated to comment
Attachment #485876 - Attachment is obsolete: true
Attachment #489769 - Attachment is obsolete: true
Attachment #491150 - Flags: review?(dolske)
Attachment #489769 - Flags: review?(dolske)
Attachment #485876 - Flags: review?(dolske)
Attachment #485876 - Flags: review?(benjamin)
Created attachment 491152 [details] [diff] [review]
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.
Attachment #491152 - Flags: review?(dtownsend)
btw: the tests pass on try
Attachment #491152 - Flags: review?(dtownsend) → review+
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.
Attachment #491150 - Flags: review?(dolske) → review+
Created attachment 491735 [details] [diff] [review]
Patch as pushed - one minor comment change
Created attachment 491736 [details] [diff] [review]
Test patch as pushed
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/31504d265266
http://hg.mozilla.org/mozilla-central/rev/7bb86e5cc3b7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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.
Status: RESOLVED → VERIFIED

Updated

7 years ago
Duplicate of this bug: 580724
Created attachment 496435 [details] [diff] [review]
1.9.2 patch
Attachment #496435 - Flags: review+
Created attachment 496437 [details] [diff] [review]
1.9.2 tests
Attachment #496437 - Flags: review+
Blocks: 570058
Attachment #496435 - Flags: approval1.9.2.14?
Comment on attachment 496435 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.14, a=dveditz for release-drivers
Attachment #496435 - Flags: approval1.9.2.14? → approval1.9.2.14+
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
status1.9.2: --- → .14-fixed
Backed out of 1.9.2 to investigate if this caused bug 633869
status1.9.2: .14-fixed → ---
Created attachment 517684 [details] [diff] [review]
Updated 1.9.2 patch

Updated

7 years ago
blocking1.9.2: --- → .16+
status1.9.2: --- → wanted
Attachment #517684 - Flags: approval1.9.2.16?
The tests that haven't landed for this bug are in bug 601518
Attachment #517684 - Flags: review+

Comment 55

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

a=LegNeato for 1.9.2.16

Updated

7 years ago
Attachment #517684 - Flags: approval1.9.2.16? → approval1.9.2.16+

Updated

7 years ago
Attachment #496435 - Flags: approval1.9.2.14+
Blocks: 639542
Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0ef1834d18f2
status1.9.2: wanted → .16-fixed
Flags: wanted1.9.0.x+
Duplicate of this bug: 654014

Comment 58

6 years ago
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)...
You need to log in before you can comment on or make changes to this bug.