Closed Bug 312010 Opened 15 years ago Closed 11 years ago

possible to start firefox when the "Software Update" dialog is running

Categories

(Toolkit :: Application Update, defect, P2)

x86
Windows XP
defect

Tracking

()

RESOLVED DUPLICATE of bug 525390
mozilla1.8.1beta2

People

(Reporter: bugzilla, Assigned: robert.strong.bugs)

References

Details

(Whiteboard: [not needed for 1.9])

Attachments

(2 files)

I got the Software Update dialog and while it was shown I was able to start
Mozilla Firefox. I dont think I should be.
A warning like this should be shown:
"Dont start Mozilla Firefox while the Software Update is running"
Not sure how we would prevent this... Maybe a temporary rename of firefox.exe? That would require some changes to the mar files and their generation... 
Unlike Linux where files in use can be updated, files in use on windows cannot be updated. So I guess if a person opens firefox while updates are being applied, the installation can get corrupted.
Flags: blocking1.8.1?
Flags: blocking-aviary2?
Let's at least look at what options we have here, since this could potentially cause a broken install.
Flags: blocking-firefox2? → blocking-firefox2+
Perhaps we could fix this by checking the value of updates/0/update.status when Firefox/Thunderbird starts up.  If the value is "applying" then we know that the updater is running.
(In reply to comment #4)
> Perhaps we could fix this by checking the value of updates/0/update.status when
> Firefox/Thunderbird starts up.  If the value is "applying" then we know that
> the updater is running.

But then firefox.exe would already be in use... W'd need to head off a launch somehow before that point, I think, if we're to be entirely safe.

Is this a windows-only problem? Because I think there are some windows reg values we could flip to keep firefox.exe from running...

We could create the following key:

HKLM\Software\Microsoft\WindowsNT\CurrentVersion\Image File Execution Options\firefox.exe

And then set the value 'Debugger' to the path of an alternate binary to execute. This value is meant to point to a debugger that will run *before* the binary is executed, but there's nothing stopping us from pointing it at a simple 'Please don't run while Updater is going' executable.

Thoughts?
That doesn't sound like a good long-term solution: not only does it affect all Firefox installations, but it will fall flat in a xulrunner world where all processes share the "xulrunner.exe" binary.

To fix this on trunk I would ideally like to see us leverage the DDE code and "FirefoxMessageWindow" which could then be handled by the updater EXE.

http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsNativeAppSupportWin.cpp#604
> But then firefox.exe would already be in use... W'd need to head off a launch
> somehow before that point, I think, if we're to be entirely safe.

Hmm... I'm confused by your comment, specifically the part about firefox.exe already being in use.  If a user runs firefox.exe, and that firefox.exe notices that an update is in process (because update.status contains the string "applying"), then it can realize that it should not be running.  Is there a race condition I'm missing?  Perhaps there is the opportunity for two firefox.exe's to race to launch the updater.  That race seems like more of an edge case to me, and it is probably less important to prevent it.  The other issue with my proposed solution is that it might leave firefox in a dead state if the updater is terminated before it can change "applying" to either success or failure.  Otherwise, I think it would work just fine.


> Is this a windows-only problem? Because I think there are some windows reg
> values we could flip to keep firefox.exe from running...

I think the problem applies to all platforms.  We don't want to launch a Firefox that is half one version and half another.
> ... but it will fall flat in a xulrunner world where all
> processes share the "xulrunner.exe" binary.

We have much more to deal with in the xulrunner world when it comes to update.


> To fix this on trunk I would ideally like to see us leverage the DDE code and
> "FirefoxMessageWindow" which could then be handled by the updater EXE.

We have to do this cross-platform.  What is wrong with a "lock" file in the application directory?  That's essentially what update.status is.  It seems like a lock file could be extended to apply to the xulrunner case.
(In reply to comment #8)
> If a user runs firefox.exe, and that firefox.exe notices
> that an update is in process (because update.status contains the string
> "applying"), then it can realize that it should not be running.

Right. I figured that we wanted to avoid that situation entirely by preventing firefox.exe from running in the first place. Obviously we could make the 'is the updater running' check happen early in the launch process, but what happens if someone tries to launch firefox.exe at the same time as the updater is patching it? I agree that this (along with your other race condition) is an edge case, and your solution would probably for most everyone. I was thinking more along the lines of preventing firefox.exe from launching at all, which would require some OS tricks.

> The other issue with my proposed solution is that it might leave firefox
> in a dead state if the updater is terminated before it can change "applying"
> to either success or failure.

Yeah, this could be an issue. We'd need some type of check to see if we'd bailed  out of launch several times... Maybe we could somehow guess that updater had failed based on the last modified time of the update.status file?
Keywords: helpwanted
Target Milestone: --- → Firefox 2 beta1
One easy solution is 
1. when patching startes, rename firefox.exe to firefox.exe.actual and patch that one instead.
2. create a temp firefox.exe that if the use lauches would present him/her with a warning: 'Firefox is currently being updated'.
3. At, the end of the patching procedure, the updater check if temp firefox.exe is running and forcedly closes it, deletes it and renames the patched firefox.exe.actual to firefox.exe.

Now for the bad idea:
On windows, if the updater fails for some reason to kill the temp firefox.exe process, it should just place a pendingrenameoperation in the registry that copies firefox.exe.actual over firefox.exe on next reboot and then prompts the user to reboot.

Again, this imo isn't an issue on linux since 'files in use' can still be updated, so only one browsing session is affected by running firefox while 'updating firefox'. subsequent reopening of firefox ais not affected so the actual installation is not affected.
At the very least, I think it'd be reasonable to move firefox.exe aside so any shortcuts to it will be temporarily unresolved.  The idea of inserting a dummy firefox.exe during the update process is neat, but it might be more work than it is worth.  I suspect that very few users try to launch Firefox while it is being patched.
Renaming the exe is really not going to work. Windows shortcuts store a bunch of metadata about files, including size/date of the target, and if you launch it after renaming the exe it will imediately find it again and change the shortcut (it will even find it in subdirectories). I think if you have a file at the target location, though, it always takes that to be the correct thing.
On windows at least, why can't we just use file locking to ensure that the executable is not readable while the update is running? 
pushing out non-critical-path bugs to b2
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Benjamin you want to take this one?
Assignee: nobody → benjamin
Schrep, for windows only?
This is an extremely poorly-tested patch. To test the theory I created a little utility to lock a file and verified that Windows won't run the program while it's locked for writing. But I haven't tested this patch in the real world because I don't know how.
Attachment #228958 - Flags: review?(darin)
Comment on attachment 228958 [details] [diff] [review]
Lock the executable while updating, rev. 1

clever :)
Attachment #228958 - Flags: review?(darin) → review+
Windows fixed on trunk. Should I keep this bug open for other platforms?
Keywords: helpwanted
maybe.. or file bugs for other platforms.  might be easier to do that for tracking purposes.
ok, fixed on windows, I'll file followups on mac/linux.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #228958 - Flags: approval1.8.1?
Attachment #228958 - Flags: approval1.8.0.6?
Waiting a couple of days to see if this causes any regressions in nightly updaters.
Keywords: qawanted
as seen on updates from 0712 -> 0713 and 0713 to 0714

double clicking the desktop icon to launch Fx while the downloaded update was applying did nothing but open an extra window of Firefox once it had launched on completion of applying the update.
Status: RESOLVED → VERIFIED
Attachment #228958 - Flags: approval1.8.1? → approval1.8.1+
Fixed on 1.8 branch.
Keywords: qawantedfixed1.8.1
*** Bug 347176 has been marked as a duplicate of this bug. ***
Comment on attachment 228958 [details] [diff] [review]
Lock the executable while updating, rev. 1

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #228958 - Flags: approval1.8.0.7? → approval1.8.0.7+
I verified this on 1.8 the other day while testing on the Windows QA machine.
Fixed on MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.7
Blocks: 354772
This seems to have caused regression bug 354772 (update doesn't apply if firefox is launched with commandline args). Backing out of MOZILLA_1_8_BRANCH per schrep.
No longer blocks: 354772
Status: VERIFIED → REOPENED
Depends on: 354772
Keywords: verified1.8.1
Resolution: FIXED → ---
Moving the blocking flags to 1.8.1.1.  Since up until 1.5.0.7 we've had this problem on branch, and the current patch causes regressions we'll have to pick this up in 1.8.1.1 
Flags: blocking1.8.1.1?
Flags: blocking-firefox2-
Flags: blocking-firefox2+
Should this also be backed out of 1.8.0.x? qawanted to see if 1.5.0.8 is hit by regression bug 354772
Keywords: qawanted
(In reply to comment #33)
> Should this also be backed out of 1.8.0.x? qawanted to see if 1.5.0.8 is hit by
> regression bug 354772
> 

Yes
Backed out of MOZILLA_1_8_0_BRANCH. Going to leave the "fixed1.8.0.7" keyword though, in case that's useful for people trying to find what changed in that release. When this gets re-fixed we can add a fixed1.8.0.9 to match the 1.8.0.9 blocking flag.  Unless someone has a better idea about how to keep this straight
Flags: blocking1.8.0.9?
Benjamin - any thoughts on the correct solution for this one?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
I do not know how to fix this bug quickly/safely since the simple technique didn't work. Checking the existence of sentinel files is probably not safe, so you'd have to create and lock a sentinel file, and then teach the app launcher to do the same.
moving to wanted, unlikely we'll get a safe fix in time for 1.8.1.1
Flags: blocking1.8.1.1+ → wanted1.8.1.x+
Keywords: fixed1.8.0.7
Flags: blocking1.8.0.9+ → wanted1.8.0.x+
Try again for 1.8.1.2, not really important to block the 1.8.0 branch (we'll take it, of course, assuming the fix is easy to port)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2+
Flags: blocking1.8.1.2+ → wanted1.8.1.x+
Priority: -- → P2
Assignee: benjamin → nobody
Status: REOPENED → NEW
After applying the Vista fix in bug 386826 this has bitten me quite bad.
Since the checked in workaround does more harm than it helps how about removing it for 1.9 until some better solution is found?
As per comment #40 I request removing the patch again.
I don't see how that should work since Windows doesn't allow to access an opened file.
Attachment #287385 - Flags: review?(darin.moz)
Attachment #287385 - Flags: review?(darin.moz) → review?(benjamin)
Attachment #287385 - Flags: review?(benjamin) → review+
Comment on attachment 287385 [details] [diff] [review]
revert bogus patch

that's a no risk change IMO
Attachment #287385 - Flags: approval1.9?
Attachment #287385 - Flags: approval1.9? → approval1.9+
Checking in updater.cpp;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v  <--  updater.cpp
new revision: 1.31; previous revision: 1.30
done
Whiteboard: [not needed for 1.9]
Duplicate of this bug: 434512
Duplicate of this bug: 439262
Product: Firefox → Toolkit
I think bug has been sufficiently qa'd as the reproduction scenario still stands (and then some). I'm removing the flag and hoping there'll be a better solution to this sometime soon.


Can we get a confirmation whether this would be nice in "1.9" ?
Keywords: qawanted
Duplicate of this bug: 512907
Depends on: 525390
This was fixed by Bug 525390... duping to that bug.
Assignee: nobody → robert.bugzilla
Status: NEW → RESOLVED
Closed: 15 years ago11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 525390
You need to log in before you can comment on or make changes to this bug.