Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 10228566
or point your web browser to
http://mondrian/10228566
to review the following code:
Change 10228566 by stevebl...@steveblock-gears1 on 2009/02/20 16:23:14 *pending*
Fixes a couple of Geolocation bugs
- A race condition in the NetworkLocationProvider when deleting the
NetworkLocationRequest object.
- Make sure that on-shot fix requests are deleted after calling back.
R=andreip
[email protected]
DELTA=40 (20 added, 4 deleted, 16 changed)
OCL=10228566
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#73
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#30
edit
40 delta lines: 20 added, 4 deleted, 16 changed
Also consider running:
g4 lint -c 10228566
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 10228566 by stevebl...@steveblock-gears1 on 2009/02/20 16:23:14 *pending*
Fixes a couple of Geolocation bugs
- A race condition in the NetworkLocationProvider when deleting the
NetworkLocationRequest object.
- Make sure that on-shot fix requests are deleted after calling back.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#73
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#30
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#73
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-02-20 17:35:45.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-02-20 17:31:54.000000000 +0000
@@ -1019,22 +1019,35 @@
// Check that the fix request still exists.
FixRequestInfo *fix_info = MaybeGetFixRequest(fix_request_id);
- if (fix_info) {
- assert(fix_info->success_callback_timer.get());
- fix_info->success_callback_timer.reset();
-
- assert(fix_info->pending_position.IsInitialized());
- if (fix_info->pending_position.IsGoodFix()) {
- if (!MakeSuccessCallback(fix_info, fix_info->pending_position)) {
- LOG(("GearsGeolocation::CallbackRequiredImpl() : JavaScript "
- "success callback failed.\n"));
- }
- } else {
- if (!MakeErrorCallback(fix_info, fix_info->pending_position)) {
- LOG(("GearsGeolocation::CallbackRequiredImpl() : JavaScript "
- "error callback failed.\n"));
- }
- }
+ if (!fix_info) {
+ return;
+ }
+
+ if (!fix_info->repeats) {
+ // Remove the fix request from our map, so that position updates which
occur
+ // while the callback to JavaScript is in process do not trigger handling
+ // for this fix request.
+ RemoveFixRequest(fix_request_id);
+ }
+
+ assert(fix_info->success_callback_timer.get());
+ fix_info->success_callback_timer.reset();
+
+ assert(fix_info->pending_position.IsInitialized());
+ if (fix_info->pending_position.IsGoodFix()) {
+ if (!MakeSuccessCallback(fix_info, fix_info->pending_position)) {
+ LOG(("GearsGeolocation::CallbackRequiredImpl() : JavaScript "
+ "success callback failed.\n"));
+ }
+ } else {
+ if (!MakeErrorCallback(fix_info, fix_info->pending_position)) {
+ LOG(("GearsGeolocation::CallbackRequiredImpl() : JavaScript "
+ "error callback failed.\n"));
+ }
+ }
+
+ if (!fix_info->repeats) {
+ DeleteFixRequest(fix_info);
}
}
====
//depot/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc#30
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
2009-02-20 17:35:45.000000000 +0000
+++
googleclient/gears/opensource/gears/geolocation/network_location_provider.cc
2009-02-20 12:55:47.000000000 +0000
@@ -80,14 +80,16 @@
}
NetworkLocationProvider::~NetworkLocationProvider() {
- if (request_) {
- request_->StopThreadAndDelete();
- }
-
// Shut down the worker thread
is_shutting_down_ = true;
thread_notification_event_.Signal();
Join();
+
+ // Must keep the request around until our worker thread has stopped.
+ if (request_) {
+ request_->StopThreadAndDelete();
+ request_ = NULL;
+ }
radio_data_provider_->Unregister(this);
wifi_data_provider_->Unregister(this);
@@ -373,6 +375,7 @@
std::string16 access_token;
AccessTokenManager::GetInstance()->GetToken(url_, &access_token);
+ assert(request_);
return request_->MakeRequest(access_token,
radio_data_,
wifi_data_,
