2010-08-17 Mahesh Kulkarni <mahesh.kulkarni@nokia.com>
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Aug 2010 16:54:23 +0000 (16:54 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Aug 2010 16:54:23 +0000 (16:54 +0000)
        Reviewed by Steve Block.

        Geolocation preemptive permissions policy is buggy
        https://bugs.webkit.org/show_bug.cgi?id=42811

        Adds cases where permission is niether denied or granted immediately and multiple
        requests are waiting.

        * fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests-expected.txt: Added.
        * fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html: Added.
        * fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests-expected.txt: Added.
        * fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html: Added.
        * fast/dom/Geolocation/script-tests/delayed-permission-allowed-for-multiple-requests.js: Added.
        (allowPermission):
        ():
        (maybeFinishTest):
        * fast/dom/Geolocation/script-tests/delayed-permission-denied-for-multiple-requests.js: Added.
        (denyPermission):
        (maybeFinishTest):
        * platform/gtk/Skipped:
        * platform/qt/Skipped:
2010-08-17  Mahesh Kulkarni  <mahesh.kulkarni@nokia.com>

        Reviewed by Steve Block.

        Geolocation preemptive permissions policy is buggy
        https://bugs.webkit.org/show_bug.cgi?id=42811

        While waiting for permission, m_startRequestPermissionNotifier was
        used to consider only one pending request. This patch implements a set
        m_pendingForPermissionNotifiers to maintain set of pending requests.
        When user grants/denies permission all listeners will be notified.
        Also fixed issue with hasZeroTimeout() where startTimerIfNeeded() has
        to start irrespective of permission state

        Tests: fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
               fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html

        * WebCore.pro:
        * page/Geolocation.cpp:
        (WebCore::Geolocation::startRequest):
        (WebCore::Geolocation::setIsAllowed):
        (WebCore::Geolocation::startUpdating):
        (WebCore::Geolocation::handlePendingPermissionNotifiers):
        * page/Geolocation.h:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-allowed-for-multiple-requests.js [new file with mode: 0644]
LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-denied-for-multiple-requests.js [new file with mode: 0644]
LayoutTests/platform/gtk/Skipped
LayoutTests/platform/qt/Skipped
WebCore/ChangeLog
WebCore/page/Geolocation.cpp
WebCore/page/Geolocation.h

index 93b3f0a..9c92cda 100644 (file)
@@ -1,3 +1,27 @@
+2010-08-17  Mahesh Kulkarni  <mahesh.kulkarni@nokia.com>
+
+        Reviewed by Steve Block.
+
+        Geolocation preemptive permissions policy is buggy
+        https://bugs.webkit.org/show_bug.cgi?id=42811
+
+        Adds cases where permission is niether denied or granted immediately and multiple
+        requests are waiting.
+
+        * fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests-expected.txt: Added.
+        * fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html: Added.
+        * fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests-expected.txt: Added.
+        * fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html: Added.
+        * fast/dom/Geolocation/script-tests/delayed-permission-allowed-for-multiple-requests.js: Added.
+        (allowPermission):
+        ():
+        (maybeFinishTest):
+        * fast/dom/Geolocation/script-tests/delayed-permission-denied-for-multiple-requests.js: Added.
+        (denyPermission):
+        (maybeFinishTest):
+        * platform/gtk/Skipped:
+        * platform/qt/Skipped:
+
 2010-08-09  Jeremy Orlow  <jorlow@chromium.org>
 
         Reviewed by Steve Block.
diff --git a/LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests-expected.txt b/LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests-expected.txt
new file mode 100644 (file)
index 0000000..137f495
--- /dev/null
@@ -0,0 +1,11 @@
+Tests that when multiple requests are waiting for permission, no callbacks are invoked until permission is allowed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Success callback invoked
+PASS Success callback invoked
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html b/LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
new file mode 100644 (file)
index 0000000..421a026
--- /dev/null
@@ -0,0 +1,13 @@
+<!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="script-tests/delayed-permission-allowed-for-multiple-requests.js"></script>
+<script src="../../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests-expected.txt b/LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests-expected.txt
new file mode 100644 (file)
index 0000000..8ede03c
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that when multiple requests are waiting for permission, no callbacks are invoked until permission is denied.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS error.code is error.PERMISSION_DENIED
+PASS error.message is "User denied Geolocation"
+PASS error.code is error.PERMISSION_DENIED
+PASS error.message is "User denied Geolocation"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html b/LayoutTests/fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html
new file mode 100644 (file)
index 0000000..2143094
--- /dev/null
@@ -0,0 +1,13 @@
+<!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="script-tests/delayed-permission-denied-for-multiple-requests.js"></script>
+<script src="../../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-allowed-for-multiple-requests.js b/LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-allowed-for-multiple-requests.js
new file mode 100644 (file)
index 0000000..8b02a82
--- /dev/null
@@ -0,0 +1,51 @@
+description("Tests that when multiple requests are waiting for permission, no callbacks are invoked until permission is allowed.");
+
+if (window.layoutTestController)
+    window.layoutTestController.setMockGeolocationPosition(51.478, -0.166, 100);
+
+function allowPermission() {
+    permissionSet = true;
+    if (window.layoutTestController)
+        layoutTestController.setGeolocationPermission(true);
+}
+
+var watchCallbackInvoked = false;
+var oneShotCallbackInvoked = false;
+
+navigator.geolocation.watchPosition(function() {
+    if (permissionSet) {
+        testPassed('Success callback invoked');
+        watchCallbackInvoked = true;
+        maybeFinishTest();
+        return;
+    }
+    testFailed('Success callback invoked unexpectedly');
+    finishJSTest();
+}, function(err) {
+    testFailed('Error callback invoked unexpectedly');
+    finishJSTest();
+});
+
+navigator.geolocation.getCurrentPosition(function() {
+    if (permissionSet) {
+        testPassed('Success callback invoked');
+        oneShotCallbackInvoked = true;
+        maybeFinishTest();
+        return;
+    }
+    testFailed('Success callback invoked unexpectedly');
+    finishJSTest();
+}, function(err) {
+    testFailed('Error callback invoked unexpectedly');
+    finishJSTest();
+});
+
+window.setTimeout(allowPermission, 100);
+
+function maybeFinishTest() {
+    if (watchCallbackInvoked && oneShotCallbackInvoked)
+        finishJSTest();
+}
+
+window.jsTestIsAsync = true;
+window.successfullyParsed = true;
diff --git a/LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-denied-for-multiple-requests.js b/LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-denied-for-multiple-requests.js
new file mode 100644 (file)
index 0000000..f230091
--- /dev/null
@@ -0,0 +1,55 @@
+description("Tests that when multiple requests are waiting for permission, no callbacks are invoked until permission is denied.");
+
+if (window.layoutTestController)
+    window.layoutTestController.setMockGeolocationPosition(51.478, -0.166, 100);
+
+function denyPermission() {
+    permissionSet = true;
+    if (window.layoutTestController)
+        layoutTestController.setGeolocationPermission(false);
+}
+
+var watchCallbackInvoked = false;
+var oneShotCallbackInvoked = false;
+var error;
+
+navigator.geolocation.watchPosition(function() {
+    testFailed('Success callback invoked unexpectedly');
+    finishJSTest();
+}, function(e) {
+    if (permissionSet) {
+        error = e;
+        shouldBe('error.code', 'error.PERMISSION_DENIED');
+        shouldBe('error.message', '"User denied Geolocation"');
+        watchCallbackInvoked = true;
+        maybeFinishTest();
+        return;
+    }
+    testFailed('Error callback invoked unexpectedly');
+    finishJSTest();
+});
+
+navigator.geolocation.getCurrentPosition(function() {
+    testFailed('Success callback invoked unexpectedly');
+    finishJSTest();
+}, function(e) {
+    if (permissionSet) {
+        error = e;
+        shouldBe('error.code', 'error.PERMISSION_DENIED');
+        shouldBe('error.message', '"User denied Geolocation"');
+        oneShotCallbackInvoked = true;
+        maybeFinishTest();        
+        return;
+    }
+    testFailed('Error callback invoked unexpectedly');
+    finishJSTest();
+});
+window.setTimeout(denyPermission, 100);
+
+function maybeFinishTest() {
+    if (watchCallbackInvoked && oneShotCallbackInvoked)
+        finishJSTest();
+}
+
+window.jsTestIsAsync = true;
+window.successfullyParsed = true;
index 4ebc04c..26c9c04 100644 (file)
@@ -1093,6 +1093,8 @@ fast/dom/DeviceOrientation/window-property.html
 fast/dom/Geolocation/callback-exception.html
 fast/dom/Geolocation/delayed-permission-allowed.html
 fast/dom/Geolocation/delayed-permission-denied.html
+fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
+fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html
 fast/dom/Geolocation/error.html
 fast/dom/Geolocation/multiple-requests.html
 fast/dom/Geolocation/no-page-cache.html
index 9919a97..d1adbd9 100644 (file)
@@ -5481,9 +5481,11 @@ fast/events/keydown-numpad-keys.html
 fast/events/special-key-events-in-input-text.html
 
 # [Qt] DumpRenderTree lacks functionality for new Geolocation delayed permission tests
-# https://bugs.webkit.org/show_bug.cgi?id=43478
+# https://bugs.webkit.org/show_bug.cgi?id=41364
 fast/dom/Geolocation/delayed-permission-allowed.html
 fast/dom/Geolocation/delayed-permission-denied.html
+fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
+fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html
 
 # BlodBuilder is not enabled
 http/tests/local/blob/send-data-blob.html
index 589580b..0f583e9 100644 (file)
@@ -1,3 +1,28 @@
+2010-08-17  Mahesh Kulkarni  <mahesh.kulkarni@nokia.com>
+
+        Reviewed by Steve Block.
+
+        Geolocation preemptive permissions policy is buggy
+        https://bugs.webkit.org/show_bug.cgi?id=42811
+
+        While waiting for permission, m_startRequestPermissionNotifier was 
+        used to consider only one pending request. This patch implements a set
+        m_pendingForPermissionNotifiers to maintain set of pending requests. 
+        When user grants/denies permission all listeners will be notified. 
+        Also fixed issue with hasZeroTimeout() where startTimerIfNeeded() has
+        to start irrespective of permission state
+
+        Tests: fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
+               fast/dom/Geolocation/delayed-permission-denied-for-multiple-requests.html
+
+        * WebCore.pro:
+        * page/Geolocation.cpp:
+        (WebCore::Geolocation::startRequest):
+        (WebCore::Geolocation::setIsAllowed):
+        (WebCore::Geolocation::startUpdating):
+        (WebCore::Geolocation::handlePendingPermissionNotifiers):
+        * page/Geolocation.h:
+
 2010-08-11  Jeremy Orlow  <jorlow@chromium.org>
 
         Beginnings of IndexedDB persistance + IDBDatabase.description fleshed out
index ce034f3..b8f1e29 100644 (file)
@@ -293,13 +293,18 @@ PassRefPtr<Geolocation::GeoNotifier> Geolocation::startRequest(PassRefPtr<Positi
         notifier->setFatalError(PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage));
     else if (haveSuitableCachedPosition(notifier->m_options.get()))
         notifier->setUseCachedPosition();
-    else if (notifier->hasZeroTimeout() || startUpdating(notifier.get())) {
+    else if (notifier->hasZeroTimeout())
+        notifier->startTimerIfNeeded();
 #if USE(PREEMPT_GEOLOCATION_PERMISSION)
-        // Only start timer if we're not waiting for user permission.
-        if (!m_startRequestPermissionNotifier)
-#endif            
-            notifier->startTimerIfNeeded();
-    } else
+    else if (!isAllowed()) {
+        // if we don't yet have permission, request for permission before calling startUpdating()
+        m_pendingForPermissionNotifiers.add(notifier);
+        requestPermission();
+    }
+#endif
+    else if (startUpdating(notifier.get()))
+        notifier->startTimerIfNeeded();
+    else
         notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, failedToStartServiceErrorMessage));
 
     return notifier.release();
@@ -416,28 +421,10 @@ void Geolocation::setIsAllowed(bool allowed)
     m_allowGeolocation = allowed ? Yes : No;
     
 #if USE(PREEMPT_GEOLOCATION_PERMISSION)
-    if (m_startRequestPermissionNotifier) {
-        if (isAllowed()) {
-            // Permission request was made during the startUpdating process
-            m_startRequestPermissionNotifier->startTimerIfNeeded();
-            // The notifier is always ref'ed by m_oneShots or m_watchers.
-            GeoNotifier* notifier = m_startRequestPermissionNotifier.get();
-            m_startRequestPermissionNotifier = 0;
-#if ENABLE(CLIENT_BASED_GEOLOCATION)
-            if (!m_frame)
-                return;
-            Page* page = m_frame->page();
-            if (!page)
-                return;
-            page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy());
-#else
-            // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy
-#endif
-        } else {
-            m_startRequestPermissionNotifier->setFatalError(PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage));
-            m_oneShots.add(m_startRequestPermissionNotifier);
-            m_startRequestPermissionNotifier = 0;
-        }
+    // Permission request was made during the startRequest process
+    if (!m_pendingForPermissionNotifiers.isEmpty()) {
+        handlePendingPermissionNotifiers();
+        m_pendingForPermissionNotifiers.clear();
         return;
     }
 #endif
@@ -649,14 +636,6 @@ void Geolocation::geolocationServiceErrorOccurred(GeolocationService* service)
 
 bool Geolocation::startUpdating(GeoNotifier* notifier)
 {
-#if USE(PREEMPT_GEOLOCATION_PERMISSION)
-    if (!isAllowed()) {
-        m_startRequestPermissionNotifier = notifier;
-        requestPermission();
-        return true;
-    }
-#endif
-
 #if ENABLE(CLIENT_BASED_GEOLOCATION)
     if (!m_frame)
         return false;
@@ -689,6 +668,38 @@ void Geolocation::stopUpdating()
 
 }
 
+#if USE(PREEMPT_GEOLOCATION_PERMISSION)
+void Geolocation::handlePendingPermissionNotifiers()
+{
+#if ENABLE(CLIENT_BASED_GEOLOCATION)
+    if (!m_frame)
+        return;
+    Page* page = m_frame->page();
+    if (!page)
+        return;
+#endif
+
+    // While we iterate through the list, we need not worry about list being modified as the permission 
+    // is already set to Yes/No and no new listeners will be added to the pending list
+    GeoNotifierSet::const_iterator end = m_pendingForPermissionNotifiers.end();
+    for (GeoNotifierSet::const_iterator iter = m_pendingForPermissionNotifiers.begin(); iter != end; ++iter) {
+        GeoNotifier* notifier = iter->get();
+
+        if (isAllowed()) {
+            // start all pending notification requests as permission granted.
+            // The notifier is always ref'ed by m_oneShots or m_watchers.
+#if ENABLE(CLIENT_BASED_GEOLOCATION)
+            notifier->startTimerIfNeeded();
+            page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy());
+#else
+            // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy
+#endif
+        } else
+            notifier->setFatalError(PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage));
+    }
+}
+#endif
+
 } // namespace WebCore
 
 #else
index 5f1e4df..2b60922 100644 (file)
@@ -147,6 +147,10 @@ private:
     bool startUpdating(GeoNotifier*);
     void stopUpdating();
 
+#if USE(PREEMPT_GEOLOCATION_PERMISSION)
+    void handlePendingPermissionNotifiers();
+#endif
+
 #if !ENABLE(CLIENT_BASED_GEOLOCATION) && ENABLE(GEOLOCATION)
     // GeolocationServiceClient
     virtual void geolocationServicePositionChanged(GeolocationService*);
@@ -170,7 +174,7 @@ private:
     OwnPtr<GeolocationService> m_service;
 #endif
 #if USE(PREEMPT_GEOLOCATION_PERMISSION)
-    RefPtr<GeoNotifier> m_startRequestPermissionNotifier;
+    GeoNotifierSet m_pendingForPermissionNotifiers;
 #endif
     RefPtr<Geoposition> m_lastPosition;