Bug 42811 – Geolocation preemptive permissions policy is buggy

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

The Geolocation preemptive permissions policy has a number of bugs.
- We need to handle the case of multiple requests being made before permission is granted/denied. This so that when permission is granted, we can start the timers on all the waiting notifiers, and pass all their enablehighAccuracy flags to GeolocationService::startUpdating() or GeolocationController::addObserver(). This could be achieved by making m_startRequestPermissionNotifier a set.
- When permission is denied, we need to set a fatal error on all current notifiers, not just that pointed to by m_startRequestPermissionNotifier.
- When permission is denied, we should not add m_startRequestPermissionNotifier to m_oneShots, as the notifier is already in m_oneShots or m_watchers and we don't know that it's a one-shot request anyway.

[Mahesh Kulkarni]

Created attachment 62572 [details]
patch

maintains set of pending permission notifiers and starts service for non client based ports. No new tests added (?). layout tests for geolocations are passing with and without preemptive enabled.

[Steve Block]

Comment on attachment 62572 [details]
patch

> ChangeLog
> +          No new test content added
I think this warrants a new test. Unfortunately I think that requires a fix to the Mac LayoutTestController which is being done in Bug 40002. I'll try to get that bug fixed soon.

> Geolocation.cpp: 415
> page->geolocationController()->addObserver(this);
This should be in the above loop. Can you also add a FIXME to pass options to the client, as at line 635.

> Geolocation.cpp: 417
> // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy
This patch should just fix this bug - the problems with the existing code. Let's leave adding the new code for !CLIENT_BASED && PREEMPT for Bug 42027.

[Mahesh Kulkarni]

(In reply to comment #2)

Thanks for the review Steve. 

> (From update of attachment 62572 [details])
> > ChangeLog
> > +          No new test content added
> I think this warrants a new test. Unfortunately I think that requires a fix to the Mac LayoutTestController which is being done in Bug 40002. I'll try to get that bug fixed soon.

Ok few new test cases for multiple notifiers waiting permission. Do you think its good idea to raise a different bug for these new cases and mark blocked by 40002?

> 
> > Geolocation.cpp: 415
> > page->geolocationController()->addObserver(this);
> This should be in the above loop. Can you also add a FIXME to pass options to the client, as at line 635.
> 

ok, so its number of listeners for controller. Will update this in patch.

> > Geolocation.cpp: 417
> > // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy
> This patch should just fix this bug - the problems with the existing code. Let's leave adding the new code for !CLIENT_BASED && PREEMPT for Bug 42027.

ok make sense. Will take care of this

[Steve Block]

> Ok few new test cases for multiple notifiers waiting permission. Do you think
> its good idea to raise a different bug for these new cases and mark blocked by
> 40002?
No, I think the tests should be part of this patch. I'll work on getting Bug 40002 fixed asap. I've marked this bug as blocked by it.

[Mahesh Kulkarni]

Created attachment 63822 [details]
patch

This patch implements a set, m_pendingForPermissionNotifiers to maintain set of pending requests. When user grands/denies permission all listener's will be notified. Added couple of new layout test cases when multiple requests are waiting for user permission.

[Steve Block]

Comment on attachment 63822 [details]
patch

WebCore/ChangeLog:11
 +          When user grands/denies permission all listener's will be notified. 
s/grands/grants

WebCore/ChangeLog:6
 +          Geolocation preemptive permissions policy is buggy
Should be bug title, then URL on line below

WebCore/page/Geolocation.cpp:420
 +              // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy
Indentation

WebCore/page/Geolocation.cpp:417
 +                      return;
It seems odd to have this test in the loop. I think we can early-out before the loop if no frame is present. Alternatively, could you call startUpdating()?

WebCore/page/Geolocation.cpp:628
 +          m_pendingForPermissionNotifiers.add(notifier);
It might help readability to add an ASSERT(!isDenied()) above this line.

WebCore/page/Geolocation.cpp:425
 +              for (GeoNotifierSet::const_iterator iter = m_pendingForPermissionNotifiers.begin(); iter != end; ++iter) {
Would it be cleaner to use a single loop, with the 'if (isAllowed())' inside the loop?

WebCore/page/Geolocation.cpp:418
 +                  page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy());
This is incorrect. We should only add an observer / start the service if the request does not have a zero timeout. Also, if adding an observer / starting the service fails, we need to set a fatal error. See startRequest(). Perhaps we could share some code from that method?

LayoutTests/fast/dom/Geolocation/script-tests/delayed-multiple-permissions-allowed.js:1
 +  description("Tests that when a position is available, no callbacks are invoked until permission is allowed.");
Mention that here we test with multiple queued requests, to distinguish this from delayed-permission-allowed.html

[Mahesh Kulkarni]

(In reply to comment #6)
Thanks for review. 

> WebCore/page/Geolocation.cpp:417
>  +                      return;
> It seems odd to have this test in the loop. I think we can early-out before the loop if no frame is present. Alternatively, could you call startUpdating()?

StartUpdating() for PREEMPT && !CLIENT? will be covered in bug 42027.


> WebCore/page/Geolocation.cpp:425
>  +              for (GeoNotifierSet::const_iterator iter = m_pendingForPermissionNotifiers.begin(); iter != end; ++iter) {
> Would it be cleaner to use a single loop, with the 'if (isAllowed())' inside the loop?

That would have repeated isAllowed() check which we could avoid. I was thinking to write a helper function to move all code under if(isAllowed()) and make it more cleaner to read? 

> 
> WebCore/page/Geolocation.cpp:418
>  +                  page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy());
> This is incorrect. We should only add an observer / start the service if the request does not have a zero timeout. Also, if adding an observer / starting the service fails, we need to set a fatal error. See startRequest(). Perhaps we could share some code from that method?
> 

A listener is appended to the m_pendingForPermissionNotifiers queue if listener doesn't have zero timeout. Perhaps we can move 
if (m_pendingForPermissionNotifiers.isEmpty()) to startUpdating() and add permission error's to GeoNotifier::timerFired() function?

[Steve Block]

> StartUpdating() for PREEMPT && !CLIENT? will be covered in bug 42027.
I meant Geolocation::startUpdating(). Either way, I think it's neater to early-out.

> A listener is appended to the m_pendingForPermissionNotifiers queue if listener
> doesn't have zero timeout.
Yes, you're right. Perhaps add a comment/ASSERT in startUpdating() or setIsAllowed() to make this clear.

> Perhaps we can move 
> if (m_pendingForPermissionNotifiers.isEmpty()) to startUpdating() and add
> permission error's to GeoNotifier::timerFired() function?
I'm not exactly sure what you mean but I'd be happy to look at a patch.

[Mahesh Kulkarni]

Created attachment 63997 [details]
patch

Incorporates changes as per comment #6

- when notifier->hasZeroTimeout() is set, immediate call back has to be given

- permission pending notifiers will be added before startUpdating() called to make starRequest() more readble. 
  This also means that when makeCachedPositionCallbacks() calls startupdating() no redundant permission checks happen

- added handlePendingPermissionNotifiers() to make setIsAllowed() more readable. 

- m_service->startUpdate() is called in case of !CLIENT_BASED_GEOLOCATION && PREEMPT_GEOLOCATION_PERMISSION clients on successful permission set.
  We could add a layout test for this but that would mean we have to enable PREEMPT_GEOLOCATION_PERMISSION for MockGeolocationService. Please comment.

[Steve Block]

Comment on attachment 63997 [details]
patch

Thanks for the patch Mahesh. In general, I like the new structure. A few comments below.

WebCore/page/Geolocation.cpp:284
 +      // Only start update if we're not waiting for user permission.
The indentation here is wrong. Maybe move the comment inside the 'else if' and reword as 'If we don't yet have permission, request it now rather than calling startUpdating().'

WebCore/page/Geolocation.cpp:286
 +          ASSERT(!isDenied());
There's probably no need for this now that we test for isDenied() just above.

WebCore/page/Geolocation.cpp:410
 +      // Permission request was made during the startUpdating process
This comment is no longer accurate

WebCore/page/Geolocation.cpp:662
 +              // start all pending notification requests as permission granted.
This comment and the one above apply to both the client-based and non-client based code, so should be above the #if

WebCore/page/Geolocation.cpp:664
 +                  notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, failedToStartServiceErrorMessage));
Let's leave the non-client-based implementation for Bug 42027.

WebCore/page/Geolocation.cpp:655
 +          notifier->startTimerIfNeeded();
We only need to start the timer if permission is allowed and if we successfully start the service/add the observer.

[Mahesh Kulkarni]

Created attachment 64027 [details]
patch

Incorporated changes from comment #10

[Steve Block]

Comment on attachment 64027 [details]
patch

WebCore/page/Geolocation.cpp:661
 +  #endif
You could leave the TODO in place for the non-client-based impl if you like.

WebCore/page/Geolocation.cpp:651
 +      GeoNotifierSet::const_iterator end = m_pendingForPermissionNotifiers.end();
It might be worth adding a comment that we don't need to protect against the set being modified while we iterate through it because any reentrant calls from JS callbacks to getCurrentPosition()/watchPosition() will see the permission as having been set, so won't add notifiers to the pending set.

[Mahesh Kulkarni]

Created attachment 64094 [details]
patch

Thanks Steve for r+ 

I don't have commit rights so submitting the patch again with changes as per comment #12. Also if you could r+ commit queue for it to pick up and land patch automatically.

[Steve Block]

Comment on attachment 64094 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 64094 [details]
patch

Rejecting patch 64094 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20831 test cases.
fast/dom/Geolocation/delayed-multiple-permissions-allowed.html -> failed

Exiting early after 1 failures. 6591 tests run.
128.40s total testing time

6590 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3756076

[Eric Seidel (no email)]

Comment on attachment 64027 [details]
patch

Cleared Steve Block's review+ from obsolete attachment 64027 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mahesh Kulkarni]

Created attachment 64421 [details]
patch

test case description was mismatching in *expected.txt and js file. Which was the reason for commit bot automated testing to fail.

[Steve Block]

Comment on attachment 64421 [details]
patch

This patch is missing ChangeLog entries

LayoutTests/fast/dom/Geolocation/script-tests/delayed-multiple-permissions-allowed.js:1
 +  description("Tests that when multiple positions are available, no callbacks are invoked until permission is allowed.");
This is slightly misleading. There aren't multiple positions, there are multiple requests in progress, all awaiting permission. Maybe the test should be named delayed-permission-allowed-multiple-requests?

[Mahesh Kulkarni]

Created attachment 64483 [details]
patch

Incorporate changes as per comment 18. Please review

[Steve Block]

Comment on attachment 64483 [details]
patch

This is still missing ChangeLogs

LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html:10
 +  <script src="script-tests/delayed-permission-allowed-for-multiple-requests.html"></script>
This should be the .js file. Did you run the tests locally?

LayoutTests/platform/gtk/Skipped:1095
 +  fast/dom/Geolocation/delayed-multiple-permissions-denied.html
Needs updating

LayoutTests/platform/qt/Skipped:5497
 +  fast/dom/Geolocation/delayed-multiple-permissions-denied.html
Needs updating

[Mahesh Kulkarni]

(In reply to comment #20)
> (From update of attachment 64483 [details])
> This is still missing ChangeLogs
> 
> LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html:10
>  +  <script src="script-tests/delayed-permission-allowed-for-multiple-requests.html"></script>
> This should be the .js file. Did you run the tests locally?
> 
No :( I had that machine formatted. Apologies for the trouble. Will upload the tested one in a min

[Mahesh Kulkarni]

Created attachment 64555 [details]
patch

Renamed and tested new layout cases on Qt. But qt DMR fix will be checked-in through bug 41364. Skipping the new cases for now

[Mahesh Kulkarni]

(In reply to comment #22)
> Created an attachment (id=64555) [details]
> patch
> 
> Renamed and tested new layout cases on Qt. But qt DMR fix will be checked-in through bug 41364. Skipping the new cases for now

Spelling mistake. Qt s/DMR/DRT

[Eric Seidel (no email)]

Comment on attachment 64094 [details]
patch

Cleared Steve Block's review+ from obsolete attachment 64094 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Steve Block]

Comment on attachment 64555 [details]
patch

WebCore/ChangeLog:8
 +           While waiting for permission, m_startRequestPermissionNotifier was 
Indentation

WebCore/ChangeLog:11
 +          When user grants/denies permission all listener's will be notified. 
apostrophe

[Mahesh Kulkarni]

Created attachment 64579 [details]
patch

incorporated above comments.

[Steve Block]

Comment on attachment 64579 [details]
patch

r=me

Thanks for making all the changes!

[WebKit Commit Bot]

Comment on attachment 64579 [details]
patch

Clearing flags on attachment: 64579

Committed r65511: <http://trac.webkit.org/changeset/65511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

The new tests fail on Chromium because the Chromium DRT lacks the functionality for these delayed permission tests. See bug 43480

A failing test expectation was added in http://trac.webkit.org/changeset/65535

[Mahesh Kulkarni]

(In reply to comment #27)
> (From update of attachment 64579 [details])
> r=me
> 
> Thanks for making all the changes!

Thanks for the review Steve.

[Mahesh Kulkarni]

(In reply to comment #27)
> (From update of attachment 64579 [details])
> r=me
> 
> Thanks for making all the changes!

Thanks for the review Steve. And Johnny for fixing chromium test failures :)

[Simon Hausmann]

Revision r65511 cherry-picked into qtwebkit-2.1 with commit e3bec9eeccf5d1ebcc6d4c3ddd23df19fc9f7595