<https://bugs.webkit.org/show_bug.cgi?id=33816>
authorsfalken@apple.com <sfalken@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jan 2010 01:05:47 +0000 (01:05 +0000)
committersfalken@apple.com <sfalken@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jan 2010 01:05:47 +0000 (01:05 +0000)
Crashes in Geolocation code due to refcounting, observer balance issues.

Reviewed by Sam Weinig.

Hold a ref to the GeoNotifier while dispatching a callback. The code was
copying a data member to avoid accessing a freed this ptr, but was still
using the this ptr.

Geolocation::removeObserver calls are not always balanced with addObserver.
Instead of asserting and continuing, don't try to remove non-existant
observers.

* page/Geolocation.cpp:
(WebCore::Geolocation::GeoNotifier::timerFired): Protect notifier.
* page/GeolocationController.cpp:
(WebCore::GeolocationController::removeObserver): Change ASSERT into an if with early return.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@53441 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/page/Geolocation.cpp
WebCore/page/GeolocationController.cpp

index 537081c..91864fa 100644 (file)
@@ -1,3 +1,23 @@
+2010-01-18  Steve Falkenburg  <sfalken@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        <https://bugs.webkit.org/show_bug.cgi?id=33816>        
+        Crashes in Geolocation code due to refcounting, observer balance issues.
+        
+        Hold a ref to the GeoNotifier while dispatching a callback. The code was
+        copying a data member to avoid accessing a freed this ptr, but was still
+        using the this ptr.
+        
+        Geolocation::removeObserver calls are not always balanced with addObserver.
+        Instead of asserting and continuing, don't try to remove non-existant
+        observers.
+
+        * page/Geolocation.cpp:
+        (WebCore::Geolocation::GeoNotifier::timerFired): Protect notifier.
+        * page/GeolocationController.cpp:
+        (WebCore::GeolocationController::removeObserver): Change ASSERT into an if with early return.
+
 2010-01-18  Alexey Proskuryakov  <ap@apple.com>
 
         Reviewed by Darin Adler.
index 1b4ce49..5ff65df 100644 (file)
@@ -107,15 +107,15 @@ void Geolocation::GeoNotifier::timerFired(Timer<GeoNotifier>*)
 {
     m_timer.stop();
 
-    // Cache our pointer to the Geolocation object, as this GeoNotifier object
+    // Protect this GeoNotifier object, since it
     // could be deleted by a call to clearWatch in a callback.
-    Geolocation* geolocation = m_geolocation;
+    RefPtr<GeoNotifier> protect(this);
 
     if (m_fatalError) {
         if (m_errorCallback)
             m_errorCallback->handleEvent(m_fatalError.get());
         // This will cause this notifier to be deleted.
-        geolocation->fatalErrorOccurred(this);
+        m_geolocation->fatalErrorOccurred(this);
         return;
     }
 
@@ -123,7 +123,7 @@ void Geolocation::GeoNotifier::timerFired(Timer<GeoNotifier>*)
         RefPtr<PositionError> error = PositionError::create(PositionError::TIMEOUT, "Timeout expired");
         m_errorCallback->handleEvent(error.get());
     }
-    geolocation->requestTimedOut(this);
+    m_geolocation->requestTimedOut(this);
 }
 
 void Geolocation::Watchers::set(int id, PassRefPtr<GeoNotifier> prpNotifier)
index 44eba6e..968e854 100644 (file)
@@ -54,7 +54,8 @@ void GeolocationController::addObserver(Geolocation* observer)
 
 void GeolocationController::removeObserver(Geolocation* observer)
 {
-    ASSERT(m_observers.contains(observer));
+    if (!m_observers.contains(observer))
+        return;
 
     m_observers.remove(observer);
     if (m_observers.isEmpty())