WebCore: Geolocation does not correctly handle reentrant calls from callbacks.
authorbenm@google.com <benm@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Sep 2009 17:16:39 +0000 (17:16 +0000)
committerbenm@google.com <benm@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Sep 2009 17:16:39 +0000 (17:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=29040

Patch by Steve Block <steveblock@google.com> on 2009-09-10
Reviewed by Darin Adler.

Tests: fast/dom/Geolocation/reentrant-error.html
       fast/dom/Geolocation/reentrant-success.html

* page/Geolocation.cpp: Modified.
(WebCore::Geolocation::handleError): Modified. Call sendError directly, clearing notifier lists before making callback.
(WebCore::Geolocation::makeSuccessCallbacks): Modified. Call sendPosition directly, clearing notifier lists before making callback.
* page/Geolocation.h: Modified. Deleted sendErrorToXX and sendPositionToXXX methods.

LayoutTests: Geolocation does not correctly handle reentrant calls from callbacks.
https://bugs.webkit.org/show_bug.cgi?id=29040

Patch by Steve Block <steveblock@google.com> on 2009-09-09
Reviewed by Darin Adler.

* fast/dom/Geolocation/resources/reentrant-error.js: Added. Tests that reentrant calls from the error callback are OK.
* fast/dom/Geolocation/reentrant-error.html: Added. Wrapper for above test.
* fast/dom/Geolocation/reentrant-error-expected.txt: Added. Expected result for above test.
* fast/dom/Geolocation/resources/reentrant-success.js: Added. Tests that reentrant calls from the success callback are OK.
* fast/dom/Geolocation/reentrant-success.html: Added. Wrapper for above test.
* fast/dom/Geolocation/reentrant-success-expected.txt: Added. Expected result for above test.
* platform/gtk/Skipped: Modified. Skips above tests.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/Geolocation/reentrant-error-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/reentrant-error.html [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/reentrant-success-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/reentrant-success.html [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/resources/reentrant-error.js [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/resources/reentrant-success.js [new file with mode: 0644]
LayoutTests/platform/gtk/Skipped
WebCore/ChangeLog
WebCore/page/Geolocation.cpp
WebCore/page/Geolocation.h

index c294493..138100e 100644 (file)
@@ -1,3 +1,18 @@
+2009-09-09  Steve Block  <steveblock@google.com>
+
+        Reviewed by Darin Adler.
+
+        Geolocation does not correctly handle reentrant calls from callbacks.
+        https://bugs.webkit.org/show_bug.cgi?id=29040
+
+        * fast/dom/Geolocation/resources/reentrant-error.js: Added. Tests that reentrant calls from the error callback are OK.
+        * fast/dom/Geolocation/reentrant-error.html: Added. Wrapper for above test.
+        * fast/dom/Geolocation/reentrant-error-expected.txt: Added. Expected result for above test.
+        * fast/dom/Geolocation/resources/reentrant-success.js: Added. Tests that reentrant calls from the success callback are OK.
+        * fast/dom/Geolocation/reentrant-success.html: Added. Wrapper for above test.
+        * fast/dom/Geolocation/reentrant-success-expected.txt: Added. Expected result for above test.
+        * platform/gtk/Skipped: Modified. Skips above tests.
+
 2009-09-10  Erik Arvidsson  <arv@chromium.org>
 
         Reviewed by Eric Seidel.
diff --git a/LayoutTests/fast/dom/Geolocation/reentrant-error-expected.txt b/LayoutTests/fast/dom/Geolocation/reentrant-error-expected.txt
new file mode 100644 (file)
index 0000000..674b61c
--- /dev/null
@@ -0,0 +1,21 @@
+Tests that reentrant calls to Geolocation methods from the error callback are OK.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS error.code is mockCode
+PASS error.message is mockMessage
+PASS error.UNKNOWN_ERROR is 0
+PASS error.PERMISSION_DENIED is 1
+PASS error.POSITION_UNAVAILABLE is 2
+PASS error.TIMEOUT is 3
+
+PASS error.code is mockCode
+PASS error.message is mockMessage
+PASS error.UNKNOWN_ERROR is 0
+PASS error.PERMISSION_DENIED is 1
+PASS error.POSITION_UNAVAILABLE is 2
+PASS error.TIMEOUT is 3
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Geolocation/reentrant-error.html b/LayoutTests/fast/dom/Geolocation/reentrant-error.html
new file mode 100644 (file)
index 0000000..6f16e96
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../../js/resources/js-test-style.css">
+<script src="../../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/reentrant-error.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Geolocation/reentrant-success-expected.txt b/LayoutTests/fast/dom/Geolocation/reentrant-success-expected.txt
new file mode 100644 (file)
index 0000000..d6e8ff4
--- /dev/null
@@ -0,0 +1,15 @@
+Tests that reentrant calls to Geolocation methods from the success callback are OK.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS position.coords.latitude is mockLatitude
+PASS position.coords.longitude is mockLongitude
+PASS position.coords.accuracy is mockAccuracy
+
+PASS position.coords.latitude is mockLatitude
+PASS position.coords.longitude is mockLongitude
+PASS position.coords.accuracy is mockAccuracy
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Geolocation/reentrant-success.html b/LayoutTests/fast/dom/Geolocation/reentrant-success.html
new file mode 100644 (file)
index 0000000..e5b071c
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../../js/resources/js-test-style.css">
+<script src="../../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/reentrant-success.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Geolocation/resources/reentrant-error.js b/LayoutTests/fast/dom/Geolocation/resources/reentrant-error.js
new file mode 100644 (file)
index 0000000..077a51a
--- /dev/null
@@ -0,0 +1,52 @@
+description("Tests that reentrant calls to Geolocation methods from the error callback are OK.");
+
+var mockCode = 0;
+var mockMessage = 'test';
+
+window.layoutTestController.setMockGeolocationError(mockCode, mockMessage);
+
+var error;
+var errorCallbackInvoked = false;
+navigator.geolocation.getCurrentPosition(function(p) {
+    testFailed('Success callback invoked unexpectedly');
+    window.layoutTestController.notifyDone();
+}, function(e) {
+    if (errorCallbackInvoked) {
+        testFailed('Error callback invoked unexpectedly');
+        window.layoutTestController.notifyDone();
+    }
+    errorCallbackInvoked = true;
+
+    error = e;
+    shouldBe('error.code', 'mockCode');
+    shouldBe('error.message', 'mockMessage');
+    shouldBe('error.UNKNOWN_ERROR', '0');
+    shouldBe('error.PERMISSION_DENIED', '1');
+    shouldBe('error.POSITION_UNAVAILABLE', '2');
+    shouldBe('error.TIMEOUT', '3');
+    debug('');
+    continueTest();
+});
+
+function continueTest() {
+    mockCode += 1;
+    mockMessage += ' repeat';
+
+    window.layoutTestController.setMockGeolocationError(mockCode, mockMessage);
+
+    navigator.geolocation.getCurrentPosition(function(p) {
+        testFailed('Success callback invoked unexpectedly');
+        window.layoutTestController.notifyDone();
+    }, function(e) {
+        error = e;
+        shouldBe('error.code', 'mockCode');
+        shouldBe('error.message', 'mockMessage');
+        shouldBe('error.UNKNOWN_ERROR', '0');
+        shouldBe('error.PERMISSION_DENIED', '1');
+        shouldBe('error.POSITION_UNAVAILABLE', '2');
+        shouldBe('error.TIMEOUT', '3');
+        debug('<br /><span class="pass">TEST COMPLETE</span>');
+        window.layoutTestController.notifyDone();
+    });
+}
+window.layoutTestController.waitUntilDone();
diff --git a/LayoutTests/fast/dom/Geolocation/resources/reentrant-success.js b/LayoutTests/fast/dom/Geolocation/resources/reentrant-success.js
new file mode 100644 (file)
index 0000000..564cc47
--- /dev/null
@@ -0,0 +1,52 @@
+description("Tests that reentrant calls to Geolocation methods from the success callback are OK.");
+
+var mockLatitude = 51.478;
+var mockLongitude = -0.166;
+var mockAccuracy = 100.0;
+
+window.layoutTestController.setGeolocationPermission(true);
+window.layoutTestController.setMockGeolocationPosition(mockLatitude,
+                                                       mockLongitude,
+                                                       mockAccuracy);
+
+var position;
+var successCallbackInvoked = false;
+navigator.geolocation.getCurrentPosition(function(p) {
+    if (successCallbackInvoked) {
+        testFailed('Success callback invoked unexpectedly');
+        window.layoutTestController.notifyDone();
+    }
+    successCallbackInvoked = true;
+
+    position = p;
+    shouldBe('position.coords.latitude', 'mockLatitude');
+    shouldBe('position.coords.longitude', 'mockLongitude');
+    shouldBe('position.coords.accuracy', 'mockAccuracy');
+    debug('');
+    continueTest();
+}, function(e) {
+    testFailed('Error callback invoked unexpectedly');
+    window.layoutTestController.notifyDone();
+});
+
+function continueTest() {
+    window.layoutTestController.setMockGeolocationPosition(++mockLatitude,
+                                                           ++mockLongitude,
+                                                           ++mockAccuracy);
+
+    navigator.geolocation.getCurrentPosition(function(p) {
+        position = p;
+        shouldBe('position.coords.latitude', 'mockLatitude');
+        shouldBe('position.coords.longitude', 'mockLongitude');
+        shouldBe('position.coords.accuracy', 'mockAccuracy');
+        debug('<br /><span class="pass">TEST COMPLETE</span>');
+        window.layoutTestController.notifyDone();
+    }, function(e) {
+        testFailed('Error callback invoked unexpectedly');
+        window.layoutTestController.notifyDone();
+    });
+}
+window.layoutTestController.waitUntilDone();
+
+
+
index 148f82b..fd3b6d4 100644 (file)
@@ -1562,6 +1562,8 @@ fast/dom/Geolocation/error.html
 fast/dom/Geolocation/permission-denied.html
 fast/dom/Geolocation/permission-denied-stops-watches.html
 fast/dom/Geolocation/position-string.html
+fast/dom/Geolocation/reentrant-error.html
+fast/dom/Geolocation/reentrant-success.html
 fast/dom/Geolocation/success.html
 fast/dom/Geolocation/timeout-zero.html
 fast/dom/Geolocation/timeout.html
index e6c6a96..233aa42 100644 (file)
@@ -1,3 +1,18 @@
+2009-09-10  Steve Block  <steveblock@google.com>
+
+        Reviewed by Darin Adler.
+
+        Geolocation does not correctly handle reentrant calls from callbacks.
+        https://bugs.webkit.org/show_bug.cgi?id=29040
+
+        Tests: fast/dom/Geolocation/reentrant-error.html
+               fast/dom/Geolocation/reentrant-success.html
+
+        * page/Geolocation.cpp: Modified.
+        (WebCore::Geolocation::handleError): Modified. Call sendError directly, clearing notifier lists before making callback.
+        (WebCore::Geolocation::makeSuccessCallbacks): Modified. Call sendPosition directly, clearing notifier lists before making callback.
+        * page/Geolocation.h: Modified. Deleted sendErrorToXX and sendPositionToXXX methods.
+
 2009-09-10  Erik Arvidsson  <arv@chromium.org>
 
         Reviewed by Eric Seidel.
index 477ac13..86127d7 100644 (file)
@@ -181,22 +181,6 @@ void Geolocation::sendError(Vector<RefPtr<GeoNotifier> >& notifiers, PositionErr
      }
 }
 
-void Geolocation::sendErrorToOneShots(PositionError* error)
-{
-    Vector<RefPtr<GeoNotifier> > copy;
-    copyToVector(m_oneShots, copy);
-
-    sendError(copy, error);
-}
-
-void Geolocation::sendErrorToWatchers(PositionError* error)
-{
-    Vector<RefPtr<GeoNotifier> > copy;
-    copyValuesToVector(m_watchers, copy);
-
-    sendError(copy, error);
-}
-
 void Geolocation::sendPosition(Vector<RefPtr<GeoNotifier> >& notifiers, Geoposition* position)
 {
     Vector<RefPtr<GeoNotifier> >::const_iterator end = notifiers.end();
@@ -208,22 +192,6 @@ void Geolocation::sendPosition(Vector<RefPtr<GeoNotifier> >& notifiers, Geoposit
     }
 }
 
-void Geolocation::sendPositionToOneShots(Geoposition* position)
-{
-    Vector<RefPtr<GeoNotifier> > copy;
-    copyToVector(m_oneShots, copy);
-    
-    sendPosition(copy, position);
-}
-
-void Geolocation::sendPositionToWatchers(Geoposition* position)
-{
-    Vector<RefPtr<GeoNotifier> > copy;
-    copyValuesToVector(m_watchers, copy);
-    
-    sendPosition(copy, position);
-}
-
 void Geolocation::stopTimer(Vector<RefPtr<GeoNotifier> >& notifiers)
 {
     Vector<RefPtr<GeoNotifier> >::const_iterator end = notifiers.end();
@@ -259,13 +227,22 @@ void Geolocation::handleError(PositionError* error)
 {
     ASSERT(error);
     
-    sendErrorToOneShots(error);    
-    sendErrorToWatchers(error);
+    Vector<RefPtr<GeoNotifier> > oneShotsCopy;
+    copyToVector(m_oneShots, oneShotsCopy);
+
+    Vector<RefPtr<GeoNotifier> > watchersCopy;
+    copyValuesToVector(m_watchers, watchersCopy);
 
+    // Clear the lists before we make the callbacks, to avoid clearing notifiers
+    // added by calls to Geolocation methods from the callbacks, and to prevent
+    // further callbacks to these notifiers.
     m_oneShots.clear();
     if (error->isFatal())
         m_watchers.clear();
 
+    sendError(oneShotsCopy, error);
+    sendError(watchersCopy, error);
+
     if (!hasListeners())
         m_service->stopUpdating();
 }
@@ -313,11 +290,20 @@ void Geolocation::makeSuccessCallbacks()
     ASSERT(m_service->lastPosition());
     ASSERT(isAllowed());
     
-    sendPositionToOneShots(m_service->lastPosition());
-    sendPositionToWatchers(m_service->lastPosition());
-        
+    Vector<RefPtr<GeoNotifier> > oneShotsCopy;
+    copyToVector(m_oneShots, oneShotsCopy);
+    
+    Vector<RefPtr<GeoNotifier> > watchersCopy;
+    copyValuesToVector(m_watchers, watchersCopy);
+    
+    // Clear the lists before we make the callbacks, to avoid clearing notifiers
+    // added by calls to Geolocation methods from the callbacks, and to prevent
+    // further callbacks to these notifiers.
     m_oneShots.clear();
 
+    sendPosition(oneShotsCopy, m_service->lastPosition());
+    sendPosition(watchersCopy, m_service->lastPosition());
+
     if (!hasListeners())
         m_service->stopUpdating();
 }
index 9fa061d..07a587f 100644 (file)
@@ -92,12 +92,7 @@ private:
     bool hasListeners() const { return !m_oneShots.isEmpty() || !m_watchers.isEmpty(); }
 
     void sendError(Vector<RefPtr<GeoNotifier> >&, PositionError*);
-    void sendErrorToOneShots(PositionError*);
-    void sendErrorToWatchers(PositionError*);
-    
     void sendPosition(Vector<RefPtr<GeoNotifier> >&, Geoposition*);
-    void sendPositionToOneShots(Geoposition*);
-    void sendPositionToWatchers(Geoposition*);
     
     static void stopTimer(Vector<RefPtr<GeoNotifier> >&);
     void stopTimersForOneShots();