Bug 26993 - Geolocation::requestPermission()
Summary: Geolocation::requestPermission()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
: 27690 (view as bug list)
Depends on:
Blocks: 27853
  Show dependency treegraph
 
Reported: 2009-07-06 09:11 PDT by Yong Li
Modified: 2009-08-12 17:14 PDT (History)
8 users (show)

See Also:


Attachments
the patch (1.34 KB, patch)
2009-07-28 12:10 PDT, Yong Li
no flags Details | Formatted Diff | Diff
Patch 2 for bug 26993 (3.35 KB, patch)
2009-08-07 07:18 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 3 for bug 26993 (3.25 KB, patch)
2009-08-07 13:48 PDT, Steve Block
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-07-06 09:11:51 PDT
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?
Comment 1 Yong Li 2009-07-28 09:15:34 PDT
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;
 }
Comment 2 Yong Li 2009-07-28 12:10:38 PDT
Created attachment 33659 [details]
the patch
Comment 3 Adam Barth 2009-07-30 14:57:32 PDT
*** Bug 27853 has been marked as a duplicate of this bug. ***
Comment 4 Adam Barth 2009-07-30 15:32:48 PDT
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...
Comment 5 Adam Barth 2009-07-30 18:29:29 PDT
Comment on attachment 33659 [details]
the patch

Let me know if you figure out a way to test this.
Comment 6 Adam Barth 2009-07-30 18:30:03 PDT
You also might consider adding more explanation to the ChangeLog.  :)
Comment 7 Yong Li 2009-07-30 19:56:46 PDT
(In reply to comment #6)
> You also might consider adding more explanation to the ChangeLog.  :)

Thanks!
Comment 8 Greg Bolsinga 2009-07-31 15:07:19 PDT
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!
Comment 9 Adam Barth 2009-07-31 23:43:28 PDT
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
Comment 10 Adam Barth 2009-07-31 23:43:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Steve Block 2009-08-06 09:01:48 PDT
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.
Comment 12 Yong Li 2009-08-06 09:08:19 PDT
(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;

...
Comment 13 Steve Block 2009-08-06 09:33:32 PDT
> 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.
Comment 14 Yong Li 2009-08-06 09:38:07 PDT
(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.
Comment 15 Steve Block 2009-08-06 09:43:20 PDT
> 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?
Comment 16 George Staikos 2009-08-06 09:45:03 PDT
reopening
Comment 17 Steve Block 2009-08-07 07:18:48 PDT
Created attachment 34270 [details]
Patch 2 for bug 26993

Fixes bug where callbacks are called twice when permissions are granted synchronously.
Comment 18 Eric Seidel (no email) 2009-08-07 12:39:14 PDT
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.
Comment 19 Steve Block 2009-08-07 12:59:37 PDT
> 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?
Comment 20 Darin Adler 2009-08-07 13:00:36 PDT
(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.
Comment 21 George Staikos 2009-08-07 13:45:16 PDT
*** Bug 27690 has been marked as a duplicate of this bug. ***
Comment 22 Steve Block 2009-08-07 13:48:48 PDT
Created attachment 34323 [details]
Patch 3 for bug 26993

Updated to use ASSERT_UNUSED.

Thanks - I wasn't aware of that macro.
Comment 23 Eric Seidel (no email) 2009-08-12 11:13:51 PDT
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! :)
Comment 24 Eric Seidel (no email) 2009-08-12 15:06:32 PDT
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
Comment 25 Eric Seidel (no email) 2009-08-12 15:08:09 PDT
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.
Comment 26 Eric Seidel (no email) 2009-08-12 15:09:02 PDT
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
Comment 27 Eric Seidel (no email) 2009-08-12 17:14:35 PDT
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.