Hello jripley,
I'd like you to do a code review. Please execute
g4 diff -c 8154379
or point your web browser to
http://mondrian/8154379
to review the following code:
Change 8154379 by [EMAIL PROTECTED] on 2008/09/02 17:47:14 *pending*
Bug fix in Geolocation arbitrator. Protects against the case where a
provider is deleted but still has callbacks queued up for processing.
R=jripley
[EMAIL PROTECTED]
DELTA=47 (30 added, 2 deleted, 15 changed)
OCL=8154379
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#46
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#20
edit
47 delta lines: 30 added, 2 deleted, 15 changed
Also consider running:
g4 lint -c 8154379
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 8154379 by [EMAIL PROTECTED] on 2008/09/02 17:47:14 *pending*
Bug fix in Geolocation arbitrator. Protects against the case where a
provider is deleted but still has callbacks queued up for processing.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#46
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#20
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#46
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-09-02 19:02:14.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-09-02 18:44:36.000000000 +0100
@@ -339,6 +339,11 @@
// LocationProviderBase::ListenerInterface implementation.
bool GearsGeolocation::LocationUpdateAvailable(LocationProviderBase *provider)
{
assert(provider);
+
+ // We check that the provider that invoked the callback is still in use, in
+ // LocationUpdateAvailableImpl. Checking here would require a mutex to guard
+ // providers_.
+
// We marshall this callback onto the JavaScript thread. This simplifies
// issuing new fix requests and calling back to JavaScript, which must be
done
// from the JavaScript thread.
@@ -527,6 +532,7 @@
void GearsGeolocation::HandleRepeatingRequestUpdate(int id,
const Position &position) {
ASSERT_SINGLE_THREAD();
+ assert(position.IsInitialized());
FixRequestInfo *fix_info = GetFixRequest(id);
assert(fix_info->repeats);
@@ -574,6 +580,7 @@
int id,
const Position &position) {
ASSERT_SINGLE_THREAD();
+ assert(position.IsInitialized());
FixRequestInfo *fix_info = GetFixRequest(id);
assert(!fix_info->repeats);
@@ -610,8 +617,27 @@
LocationProviderBase *provider) {
ASSERT_SINGLE_THREAD();
+ // Check that the provider that invoked the callback is still in use. By the
+ // time we receive this marshalled callback the provider may have been
+ // unregistered and deleted.
+ ProviderMap::iterator provider_iter = providers_.find(provider);
+ if (provider_iter == providers_.end()) {
+ return;
+ }
+
+ // Get the position from the provider.
Position position;
provider->GetPosition(&position);
+
+ // The location provider should only call us back when it has a valid
+ // position. However, it's possible that this marshalled callback is due to a
+ // previous provider, which no longer exists. If a new provider is now
+ // registered at the same address as the previous one, the above check will
+ // not detect this case and we'll call GetPosition on the new provider
without
+ // it having previously called us back.
+ if (!position.IsInitialized()) {
+ return;
+ }
// Update the last known position, which is the best position estimate we
// currently have.
@@ -633,17 +659,15 @@
// Iterate over all non-repeating fix requests of which this provider is a
// part.
- ProviderMap::iterator provider_iter = providers_.find(provider);
- if (provider_iter != providers_.end()) {
- IdList ids = provider_iter->second;
- while (!ids.empty()) {
- int id = ids.back();
- ids.pop_back();
- if (id < 0) {
- FixRequestInfoMap::const_iterator iter = fix_requests_.find(id);
- if (iter != fix_requests_.end()) {
- HandleSingleRequestUpdate(provider, id, position);
- }
+ assert(provider_iter != providers_.end());
+ IdList ids = provider_iter->second;
+ while (!ids.empty()) {
+ int id = ids.back();
+ ids.pop_back();
+ if (id < 0) {
+ FixRequestInfoMap::const_iterator iter = fix_requests_.find(id);
+ if (iter != fix_requests_.end()) {
+ HandleSingleRequestUpdate(provider, id, position);
}
}
}
@@ -672,6 +696,7 @@
const Position &position) {
ASSERT_SINGLE_THREAD();
assert(fix_info);
+ assert(position.IsInitialized());
assert(position.IsGoodFix());
scoped_ptr<JsObject> position_object(GetJsRunner()->NewObject());
@@ -707,6 +732,7 @@
const Position &position) {
ASSERT_SINGLE_THREAD();
assert(fix_info);
+ assert(position.IsInitialized());
assert(!position.IsGoodFix());
// The error callback is optional.
@@ -905,6 +931,7 @@
JsObject *position_object) {
assert(js_runner);
assert(position_object);
+ assert(position.IsInitialized());
assert(position.IsGoodFix());
bool result = true;
@@ -958,6 +985,7 @@
const Position &position,
JsObject *error_object) {
assert(error_object);
+ assert(position.IsInitialized());
assert(!position.IsGoodFix());
bool result = true;
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#20 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-09-02 19:02:14.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-09-02 18:45:00.000000000 +0100
@@ -290,16 +290,16 @@
// into a separate class.
// A map from providers to fix request IDs. We use this map when looking up
- // the single fix requests in which a provider is involved, and to track when
- // a provider is no longer involved in any fix requests and we can unregister
- // form it. See the comment in the implementation of
- // LocationUpdateAvailableImpl for a discussion of how and why request IDs
are
- // used.
+ // the single fix requests in which a provider is involved and to make sure
+ // that the provider from which a location update is received is still valid
+ // once the callback has been marshalled. See the comment in the
+ // implementation of LocationUpdateAvailableImpl for a discussion of how and
+ // why request IDs are used.
typedef std::vector<int> IdList;
typedef std::map<LocationProviderBase*, IdList> ProviderMap;
ProviderMap providers_;
- // Map from watch ID to fix request.
+ // Map from fix request ID to fix request.
typedef std::map<int, FixRequestInfo*> FixRequestInfoMap;
FixRequestInfoMap fix_requests_;
int next_single_request_id_; // Always negative
