Changeset 53441 in webkit

Timestamp:

Jan 18, 2010 5:05:47 PM (14 years ago)

Author:

sfalken@apple.com

Message:

<​https://bugs.webkit.org/show_bug.cgi?id=33816>
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.

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/Geolocation.cpp (modified) (3 diffs)
• page/GeolocationController.cpp (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/page/Geolocation.cpp

@@ -108 +108 @@
     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) {
@@ -116 +116 @@
             m_errorCallback->handleEvent(m_fatalError.get());
         // This will cause this notifier to be deleted.
-        geolocation->fatalErrorOccurred(this);
+        m_geolocation->fatalErrorOccurred(this);
         return;
     }
@@ -124 +124 @@
         m_errorCallback->handleEvent(error.get());
     }
-    geolocation->requestTimedOut(this);
+    m_geolocation->requestTimedOut(this);
 }
 

trunk/WebCore/page/GeolocationController.cpp

@@ -55 +55 @@
 void GeolocationController::removeObserver(Geolocation* observer)
 {
-    ASSERT(m_observers.contains(observer));
+    if (!m_observers.contains(observer))
+        return;
 
     m_observers.remove(observer);