Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 10966391
or point your web browser to
http://mondrian/10966391
to review the following code:
Change 10966391 by stevebl...@steveblock-gears4 on 2009/04/29 15:26:21 *pending*
Fixes a Geolocation race condition when calling back with a cached
position when timeout is zero.
R=andreip
[email protected]
DELTA=19 (7 added, 5 deleted, 7 changed)
OCL=10966391
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#76
edit
19 delta lines: 7 added, 5 deleted, 7 changed
Also consider running:
g4 lint -c 10966391
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 10966391 by stevebl...@steveblock-gears4 on 2009/04/29 15:26:21 *pending*
Fixes a Geolocation race condition when calling back with a cached
position when timeout is zero.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#76
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#76
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-04-29 15:56:08.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-04-29 15:24:14.000000000 +0100
@@ -641,16 +641,12 @@
return;
}
- // See whether we need to call back immediately with a cached position.
+ // See whether we need to call back immediately with a cached position or a
+ // cached position error.
Position position_to_call_back;
if (IsCachedPositionSuitable(info->maximum_age)) {
position_to_call_back = last_position_;
- }
-
- // See whether we need to call back immediately with a cached position error.
- if (info->providers.empty() &&
- info->timeout != 0 &&
- !IsCachedPositionSuitable(info->maximum_age)) {
+ } else if (info->providers.empty() && info->timeout != 0) {
position_to_call_back.error_code =
Position::ERROR_CODE_POSITION_UNAVAILABLE;
position_to_call_back.error_message =
@@ -676,8 +672,8 @@
}
// Invoke the callback for the cached position or error. We use a message to
- // do so asynchronously. This must be done before we start the timeout timer.
- // Note that the callback handler cancels any active providers.
+ // do so asynchronously. Note that the callback handler cancels any active
+ // providers.
if (position_to_call_back.IsInitialized()) {
assert(!info->success_callback_timer.get());
@@ -688,9 +684,15 @@
new FixRequestIdNotificationData(this, fix_request_id)));
}
- // Start the timeout timer for this fix request. Note that this handles all
- // timeouts, include immediate timeout callbacks when timeout is zero.
- StartTimeoutTimer(fix_request_id);
+ // Start the timeout timer for this fix request. If the timeout is zero and
we
+ // have a cached position or cached position error, we want to callback with
+ // the cached position, not the timeout. However, there is a potential race
+ // condition between this callback and the timer callback because callbacks
+ // are invoked asynchronously and there are no guarantees on ordering. So in
+ // this case, we do not start the timeout timer.
+ if (!(info->timeout == 0 && position_to_call_back.IsInitialized())) {
+ StartTimeoutTimer(fix_request_id);
+ }
// Set the return value if this fix repeats.
if (info->repeats) {
