Bug 26993 – Geolocation::requestPermission()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yong Li]

See the following code:

void Geolocation::requestPermission()
{
    ......

    // Ask the chrome: it maintains the geolocation challenge policy itself.
    page->chrome()->requestGeolocationPermissionForFrame(m_frame, this);
    
    m_allowGeolocation = InProgress;
}

And the only case that requestPermission() is called:

void Geolocation::geolocationServicePositionChanged(GeolocationService* service)
{
    ASSERT(service->lastPosition());
    
    requestPermission();
    if (!isAllowed())
        return;

    ......
}

This is weird. Whatever Chrome::requestGeolocationPermissionForFrame() does on the Geolocation object, m_allowGeolocation is always InProgress. Unless it calls setAllowed by a timer. So as the result, the first call to geolocationServicePositionChanged must fail, and the subsequent calls can succeed only after the ChromeClient calls setIsAllowed(true) by a timer.

Why not change it to:

    m_allowGeolocation = InProgress;
    page->chrome()->requestGeolocationPermissionForFrame(m_frame, this);

Is it any reason for it?

[Yong Li]

The patch:

--- a/WebCore/page/Geolocation.cpp
+++ b/WebCore/page/Geolocation.cpp
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2008, 2009 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -264,10 +265,10 @@ void Geolocation::requestPermission()
     if (!page)
         return;

+    m_allowGeolocation = InProgress;
+
     // Ask the chrome: it maintains the geolocation challenge policy itself.
     page->chrome()->requestGeolocationPermissionForFrame(m_frame, this);
-    
-    m_allowGeolocation = InProgress;
 }

[Yong Li]

Created attachment 33659 [details]
the patch

[Adam Barth]

*** Bug 27853 has been marked as a duplicate of this bug. ***

[Adam Barth]

Is this testable somehow?  It would be easy to test with a LayoutTest, but I'm not sure how we could test it with a LayoutTest...

[Adam Barth]

Comment on attachment 33659 [details]
the patch

Let me know if you figure out a way to test this.

[Adam Barth]

You also might consider adding more explanation to the ChangeLog.  :)

[Yong Li]

(In reply to comment #6)
> You also might consider adding more explanation to the ChangeLog.  :)

Thanks!

[Greg Bolsinga]

Were you able to test this out? I'd have to do quite a bit of work to get my port back to this asynchronous mode to test it out. Thanks!

[Adam Barth]

Comment on attachment 33659 [details]
the patch

Clearing review flag on attachment: 33659

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/page/Geolocation.cpp
Committed r46663
	M	WebCore/ChangeLog
	M	WebCore/page/Geolocation.cpp
r46663 = 652e8a6a543e021b653c175b4640f6d8fa392af3 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46663

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

If I've followed the comments correctly, this change is intended to support both synchronous and asynchronous implementations of requestGeolocationPermissionForFrame().

I think, however, that this patch introduces a bug in the case where requestGeolocationPermissionForFrame() is synchronous. setIsAllowed() calls geolocationServicePositionChanged(), so if requestGeolocationPermissionForFrame() is synchronous, any watches will be called back twice, rather than once as intended.

See https://bugs.webkit.org/show_bug.cgi?id=27690 (which is a duplicate of this bug) for a full explanation.

[Yong Li]

(In reply to comment #11)
> If I've followed the comments correctly, this change is intended to support
> both synchronous and asynchronous implementations of
> requestGeolocationPermissionForFrame().
> 
> I think, however, that this patch introduces a bug in the case where
> requestGeolocationPermissionForFrame() is synchronous. setIsAllowed() calls
> geolocationServicePositionChanged(), so if
> requestGeolocationPermissionForFrame() is synchronous, any watches will be
> called back twice, rather than once as intended.
> 
> See https://bugs.webkit.org/show_bug.cgi?id=27690 (which is a duplicate of this
> bug) for a full explanation.

I don't think it's a problem, because:

void Geolocation::requestPermission()
{
    if (m_allowGeolocation > Unknown)
        return;

...

[Steve Block]

> I don't think it's a problem, because:
I'm pretty sure it is a problem. You're right that the check on m_allowGeolocation in requestPermission() will prevent repeated calls to requestGeolocationPermissionForFrame(), but there will still be repeated callbacks to watchers.

The first callbacks are made when the call stack is ...
geolocationServicePositionChanged() -> requestPermission() -> requestGeolocationPermissionForFrame() -> setIsAllowed() -> geolocationServicePositionChanged()

The stack then unwinds to ...
geolocationServicePositionChanged()
and the callbacks are made a second time.

[Yong Li]

(In reply to comment #13)
> > I don't think it's a problem, because:
> I'm pretty sure it is a problem. You're right that the check on
> m_allowGeolocation in requestPermission() will prevent repeated calls to
> requestGeolocationPermissionForFrame(), but there will still be repeated
> callbacks to watchers.
> 
> The first callbacks are made when the call stack is ...
> geolocationServicePositionChanged() -> requestPermission() ->
> requestGeolocationPermissionForFrame() -> setIsAllowed() ->
> geolocationServicePositionChanged()
> 
> The stack then unwinds to ...
> geolocationServicePositionChanged()
> and the callbacks are made a second time.

I see. It sends current position twice.

[Steve Block]

> I see. It sends current position twice.
Indeed. I'll send a patch to fix this if somebody can review it.

Can somebody also reopen this bug and mark Bug 27690 as a duplicate?

[George Staikos]

reopening

[Steve Block]

Created attachment 34270 [details]
Patch 2 for bug 26993

Fixes bug where callbacks are called twice when permissions are granted synchronously.

[Eric Seidel (no email)]

Comment on attachment 34270 [details]
Patch 2 for bug 26993

Why do we now have an unused argument?  Seems we should at least be asserting, no?

Why shoudn't makeSuccessCallbacks take an service parameter?

r- for the unexplained unused argument.

[Steve Block]

> Why do we now have an unused argument?
The service argument passed to geolocationServicePositionChanged() should always be equal to m_service.

> Seems we should at least be asserting, no?
Ideally, yes, we'd ASSERT(service == m_service). However, since service is not used in this method (nor is m_service), using service in an ASSERT only gives an 'unused argument' build warning in opt builds.

I could ifdef the service argument in the method definition for debug builds only?

[Darin Adler]

(In reply to comment #19)
> > Seems we should at least be asserting, no?
> Ideally, yes, we'd ASSERT(service == m_service). However, since service is not
> used in this method (nor is m_service), using service in an ASSERT only gives
> an 'unused argument' build warning in opt builds.

The ASSERT_UNUSED macro is for this sort of situation.

    ASSERT_UNUSED(service, service == m_service);

The above will do the trick.

[George Staikos]

*** Bug 27690 has been marked as a duplicate of this bug. ***

[Steve Block]

Created attachment 34323 [details]
Patch 3 for bug 26993

Updated to use ASSERT_UNUSED.

Thanks - I wasn't aware of that macro.

[Eric Seidel (no email)]

Comment on attachment 34323 [details]
Patch 3 for bug 26993

Your ChangeLog shoudl have said "No new tests possible.' and explained why.  Tests are always required! :)

[Eric Seidel (no email)]

Comment on attachment 34323 [details]
Patch 3 for bug 26993

Rejecting patch 34323 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 34323 [details]
Patch 3 for bug 26993

Tests passed.  ChangeLog was out of date by the time it went to commit.  Will land manually.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/page/Geolocation.cpp
	M	WebCore/page/Geolocation.h
Committed r47152
	M	WebCore/ChangeLog
	M	WebCore/page/Geolocation.cpp
	M	WebCore/page/Geolocation.h
r47152 = 27684299a4c6bc752037f37c7391cb19a17e7c60 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47152

[Eric Seidel (no email)]

Comment on attachment 34323 [details]
Patch 3 for bug 26993

Rejecting patch 34323 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34323 from bug 26993 failed to download and apply.