Try to lock the main exe multiple time before giving up

RESOLVED FIXED in mozilla2.0b8

Status

()

Toolkit
Application Update
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b8
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(status1.9.2 .17-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Currently, the updater waits 50 milliseconds after the process exits before continuing and it is possible for the process to exit before the process image is released. Instead, the updater should try to lock the main exe multiple times before giving up.

patch coming up
Created attachment 489703 [details] [diff] [review]
patch rev1 - wait up to 250 ms which is 200 more than it currently waits
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #489703 - Flags: review?(dolske)
Comment on attachment 489703 [details] [diff] [review]
patch rev1 - wait up to 250 ms which is 200 more than it currently waits


>+    int count = 0;
>+    do {
...
>+      count++;
>+    } while (count < 5);
>+

Teensy style nit: how about this?

  int retries = 5;
  do {
  ...
  } while (--retries);


Also: Hmm. I suppose there's a downside of this increasing startup by 250ms whenever there's a pending update and the binary is legitimately in user by another user/profile. And 250ms might still be too slow in some cases (though you said this is the first you've heard of the problem with 50ms, so that's probably unlikely to be a real issue?). But offhand I can't think of a better way to do this that doesn't require some kind of complicated "are you alive?" IPC to the running process, and that's hairy for the obvious reasons.
Attachment #489703 - Flags: review?(dolske)
Attachment #489703 - Flags: review+
Attachment #489703 - Flags: approval2.0+
Created attachment 489744 [details] [diff] [review]
patch updated to comments

Though it would be nice to be able to know exactly when the image is no longer in use but I can't think of a way. The image itself is actually can still be in use after WaitForSingleObject claims it no longer is so even if an IPC mechanism was reasonable I doubt it would work in this instance. Having said that, an additional 200 ms for the edgecase seems reasonable for the case where it is in use by another user and for the normal case where it isn't in use it will be 50 ms faster when the image is released quickly.
Attachment #489703 - Attachment is obsolete: true
Attachment #489744 - Flags: review+
Comment on attachment 489744 [details] [diff] [review]
patch updated to comments

Drivers, this patch checks 5 times with a 50 ms sleep between each try for the firefox.exe file being in use before giving up instead of just waiting 50 ms and expecting the image to be released. In the worst case (which I believe is the edgecase), a user trying to apply an update when another instance of firefox.exe is running will have to wait an additional 250 ms. In the best case, a user trying to apply an update will wait 50 ms less and for the edgcase where 50 ms previously wasn't enough it will retry up to 5 times.
Attachment #489744 - Flags: approval2.0?
Attachment #489744 - Flags: approval2.0? → approval2.0+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/302e3ea21728

This is in part already tested in the test suite and the remainder can't be tested by litmus though the functionality is tested everytime someone updates so I think this is well covered.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Created attachment 517687 [details] [diff] [review]
1.9.2 patch
Comment on attachment 517687 [details] [diff] [review]
1.9.2 patch

Since will likely be taking several updater changes on 1.9.2 I'd like to get
this as well so the code is consistent with Firefox 4 / trunk.
Attachment #517687 - Flags: review+
Attachment #517687 - Flags: approval1.9.2.16?
Comment on attachment 517687 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.16, a=dveditz for release-drivers
Attachment #517687 - Flags: approval1.9.2.16? → approval1.9.2.16+
Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b4c86762986
status1.9.2: --- → .16-fixed
You need to log in before you can comment on or make changes to this bug.