611186 - Try to lock the main exe multiple time before giving up

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

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

Comment 1

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

Attachment: patch rev1 - wait up to 250 ms which is 200 more than it currently waits

Comment 2

[Justin Dolske [: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.

Comment 3

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

Attachment: 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.

Comment 4

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

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.

Comment 5

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

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.

Comment 6

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

Attachment: 1.9.2 patch

Comment 7

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

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.

Comment 8

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.2.16, a=dveditz for release-drivers

Comment 9

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

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b4c86762986