Bug 39434 - REGRESSION (r59811): Geolocation callbacks cannot be created
Summary: REGRESSION (r59811): Geolocation callbacks cannot be created
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Alexey Proskuryakov
URL: http://chatroam.com/webkit/geolocatio...
Keywords:
: 39454 39971 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-05-20 10:54 PDT by Scott Menor
Modified: 2010-06-01 12:50 PDT (History)
8 users (show)

See Also:


Attachments
proposed fix (47.64 KB, patch)
2010-05-28 15:49 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed fix (46.84 KB, patch)
2010-06-01 10:20 PDT, Alexey Proskuryakov
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Menor 2010-05-20 10:54:34 PDT
navigator.geolocation.watchPosition(positionCallback)

fails and returns 

Error: TYPE_MISMATCH_ERR: DOM Exception 17
Comment 1 Alexey Proskuryakov 2010-05-20 17:12:52 PDT
See also: bug 39454.
Comment 2 Alexey Proskuryakov 2010-05-27 11:29:42 PDT
*** Bug 39454 has been marked as a duplicate of this bug. ***
Comment 3 Steve Block 2010-05-27 13:36:03 PDT
Is this with client-based geolocation, or the original geolocation implementation, or both?
Comment 4 Alexey Proskuryakov 2010-05-27 13:47:26 PDT
This is an issue in JavaScriptCore bindings, so it's with both. I have a fix, but I need to make tests work first.
Comment 5 Alexey Proskuryakov 2010-05-28 15:49:41 PDT
Created attachment 57391 [details]
proposed fix
Comment 6 WebKit Review Bot 2010-05-28 15:53:40 PDT
Attachment 57391 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/mac/MockGeolocationProvider.h:36:  _registeredViews is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Eric Seidel (no email) 2010-05-28 15:58:44 PDT
Attachment 57391 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2654017
Comment 8 Alexey Proskuryakov 2010-05-28 16:03:38 PDT
I guess touching didn't work. Not sure how the build can be fixed without manual intervention.
Comment 9 Darin Adler 2010-05-28 16:46:38 PDT
Comment on attachment 57391 [details]
proposed fix

> +2010-05-28  Alexey Proskuryakov  <ap@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * bindings/js/JSGeolocationCustom.cpp:
> +        (WebCore::createPositionCallback):
> +        (WebCore::createPositionErrorCallback):

Extra empty change log.

>      // The spec specifies 'FunctionOnly' for this object.
> -    if (!value.inherits(&InternalFunction::info)) {
> +    // FIXME: This check disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.
> +    if (!value.inherits(&JSFunction::info)) {
>          setDOMException(exec, TYPE_MISMATCH_ERR);
>          return 0;
>      }

This sure is peculiar. We need to find out what FunctionOnly means! Normally in JavaScript we just try to call things.

> +    // FIXME: What's the reason for isGeolocationPermissionSet() existence? The delegate should always respond by either allow or deny.
> +    if (gLayoutTestController->isGeolocationPermissionSet() && gLayoutTestController->geolocationPermission())
> +        [listener allow];
> +    else
> +        [listener deny];

Maybe we want to test the asynchrony of the call at some point. The client is allowed to wait a while before doing allow or deny and we might want to test those cases. Just a guess.

> +2010-05-28  Alexey Proskuryakov  <ap@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * fast/dom/Geolocation/script-tests/error.js:
> +        * fast/dom/Geolocation/script-tests/maximum-age.js:
> +        * fast/dom/Geolocation/script-tests/reentrant-error.js:
> +        * fast/dom/Geolocation/script-tests/watch.js:
> +        * platform/gtk/Skipped:
> +        * platform/mac/Skipped:

Another excess change log here.

The Mac EWS says this fails to build.
Comment 10 Steve Block 2010-05-31 08:27:38 PDT
Comment on attachment 57391 [details]
proposed fix

WebCore/bindings/js/JSGeolocationCustom.cpp:42
 +  #endif
What has changed such that GeolocationService.h can no longer be included when CLIENT_BASED_GEOLOCATION is not defined? If the contents of the header do need to be guarded, isn't the pattern to guard the contents, not the include site?

WebKitTools/DumpRenderTree/mac/UIDelegate.mm:160
 +      // FIXME: What's the reason for isGeolocationPermissionSet() existence? The delegate should always respond by either allow or deny.
Yes, my intent was to provide the ability for the embedder to make a delayed response to a permission request. The Geolocation object allows the call to requestGeolocationPermissionForFrame() to be synchronous or asynchronous. We should probably add a layout test to test the delayed permission response.

LayoutTests/fast/dom/Geolocation/script-tests/error.js:6
 +  window.layoutTestController.setGeolocationPermission(true);
The Geolocation spec only requires that permission be obtained from the user before a position is returned to JS. If the result of the Geolocation request is an error, permission does not need to be obtained before we call back. The non-client-based Geolocation implementation implements this. I assume this means that the client-based implementation requires permission to make an error callback? Is this intentional?

LayoutTests/platform/gtk/Skipped: 
 +  fast/dom/Geolocation/timeout-zero.html
So these failures are due to JSC binding problems? Note that all of the Geolocation layout tests are passing on Android and Chromium with V8 using non-client-based Geolocation.

WebCore/bindings/js/JSGeolocationCustom.cpp:69
 +      // FIXME: This check disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.
My understanding of the spec is that it intends to allow JS function objects, but not objects with a 'handleEvent' property. See http://lists.w3.org/Archives/Public/public-geolocation/2009Jun/0173.html for the relevant discussion on the W3C Geolocation mailing list.

The change where I added this code is http://trac.webkit.org/changeset/47252 and the V8 equivalent is http://trac.webkit.org/changeset/51540
Comment 11 Steve Block 2010-06-01 06:46:13 PDT
*** Bug 39971 has been marked as a duplicate of this bug. ***
Comment 12 Yong Li 2010-06-01 08:04:48 PDT
why cannot the callbacks be InternalFunction?
Comment 13 Steve Block 2010-06-01 09:44:06 PDT
> Yes, my intent was to provide the ability for the embedder to make a delayed
> response to a permission request. The Geolocation object allows the call to
> requestGeolocationPermissionForFrame() to be synchronous or asynchronous. We
> should probably add a layout test to test the delayed permission response.
Filed Bug 40002 to track this.
Comment 14 Alexey Proskuryakov 2010-06-01 10:19:27 PDT
> What has changed such that GeolocationService.h can no longer be included when 
> CLIENT_BASED_GEOLOCATION is not defined?

It can be included, but that causes confusion. I spent a considerable amount of time figuring out that everything with "GeolocationService" in the name is completely irrelevant for client based implementation, and this simple change would have helped.

> isn't the pattern to guard the contents, not the include site?

The normal pattern is for features that can be disabled, so there is very little potential for confusion. Here, we have two separate implementations co-existing with the help of ifdefs, which is a different case.

> We should probably add a layout test to test the delayed permission response.

I changed the FIXME comment. The delayed permission code path isn't implemented in cross-platform LayoutTestController.cpp, so currently checking for permission being unset is useless though.

> The Geolocation spec only requires that permission be obtained from the user before a position is returned to JS.

I don't think that's accurate. First, this is dangerous, since this basically allows an application to test whether one's notebook is on WiFi (since that's the source of information for most notebooks), so letting failed requests go through without asking for permission would cause a minor information leak. Second, my reading of the spec is different:

"Furthermore, the implementation should always obtain the user's permission to share location before executing any of the getCurrentPosition or watchPosition steps described above. If the user grants permission, the appropriate callback must be invoked as described above. If the user denies permission, the errorCallback (if present) must be invoked with code PERMISSION_DENIED, irrespective of any other errors encountered in the above steps."

> So these failures are due to JSC binding problems?

See duplicate bug - Gtk tests were disabled due to this problem. Apparently, Android hasn't synced JSC up to r59811 yet.

> My understanding of the spec is that it intends to allow JS function objects, 
> but not objects with a 'handleEvent' property.

I see. This still leaves the question of what a "JS function object" is according to WebIDL, or why Geolocation wants to be different from the rest of the Web platform. I couldn't find an explanation of the latter in the mailing list link you provided.

As mentioned in the FIXME comment, callable objects created via JSC API (like layoutTestController.setGeolocationPermission) should probably work as callback parameters. But current WebIDL text prevents that, and it's not trivial to fix. I don't see why it's worth fixing, given that all other callback functions on the Web don't require such trickery.

> why cannot the callbacks be InternalFunction?

I don't understand this question. Are you referring to the FIXME comment I discuss above? Then yes, the trait of being callable is not relevant to what the object inherits from. In JSC, there are two ways to chack for being callable:
1) Check callData return value. That's the closest to just try and call the object, but it includes RegExp objects, which we may or may not want to exclude;
2) Ignore inheritance, and check that className is "Function". That seems to match the current WebIDL text best, but it doesn't cover objects created via API.

Here, I'm trying to fix the regression in basic case. Discussing whether we want to fix or ignore the spec should probably take place in a separate bug.
Comment 15 Alexey Proskuryakov 2010-06-01 10:20:06 PDT
Created attachment 57561 [details]
proposed fix

Removed empty ChangeLog entries, re-worded a FIXME.
Comment 16 WebKit Review Bot 2010-06-01 10:25:31 PDT
Attachment 57561 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/mac/MockGeolocationProvider.h:36:  _registeredViews is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Eric Seidel (no email) 2010-06-01 10:26:31 PDT
Attachment 57561 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2784056
Comment 18 Steve Block 2010-06-01 10:46:23 PDT
> > The Geolocation spec only requires that permission be obtained from the user before a position is returned to JS.
> 
> I don't think that's accurate.
Note the use of 'must' and 'should' in ...
'For both getCurrentPosition and watchPosition, the implementation must never invoke the successCallback without having first obtained permission from the user to share location. Furthermore, the implementation should always obtain the user's permission to share location before executing any of the getCurrentPosition or watchPosition steps described above.'

Implementers are not required to follow a 'should'. On Android we chose not to prompt the user for permission until we're sure we have a position, as the usability cost of the prompt is high.

I don't think that WebCore should enforce that permission is required to make error callbacks. If an embedder chooses to enforce this, they can do so outside of WebCore. However, updating LayoutTests to work in both cases is fine with me.

> Here, I'm trying to fix the regression in basic case. Discussing whether we
> want to fix or ignore the spec should probably take place in a separate bug.
Agreed, I was just trying to provide background. Can you file a bug for this and we can discuss with the spec authors.
Comment 19 Alexey Proskuryakov 2010-06-01 10:49:04 PDT
Committed <http://trac.webkit.org/changeset/60488>.
Comment 20 Alexey Proskuryakov 2010-06-01 11:26:23 PDT
> Implementers are not required to follow a 'should'. On Android we chose not to prompt the user for permission until we're sure we have a position, as the usability cost of the prompt is high.

I see, thanks.

> Agreed, I was just trying to provide background. Can you file a bug for this and we can discuss with the spec authors.

Filed bug 40012.
Comment 21 Yong Li 2010-06-01 12:18:50 PDT
(In reply to comment #14)
> 
> > why cannot the callbacks be InternalFunction?
> 
> I don't understand this question. Are you referring to the FIXME comment I discuss above? Then yes, the trait of being callable is not relevant to what the object inherits from. In JSC, there are two ways to chack for being callable:
> 1) Check callData return value. That's the closest to just try and call the object, but it includes RegExp objects, which we may or may not want to exclude;
> 2) Ignore inheritance, and check that className is "Function". That seems to match the current WebIDL text best, but it doesn't cover objects created via API.
> 

9   if (!value.inherits(&InternalFunction::info)) {
 52 // FIXME: This check disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.
 53 if (!value.inherits(&JSFunction::info)) { 

Yes, why not just do both checks?

if (!value.inherits(&InternalFunction::info) && !value.inherits(&JSFunction::info)) {
Comment 22 Alexey Proskuryakov 2010-06-01 12:37:33 PDT
> Yes, why not just do both checks?

I'm not sure how that would help - a JSObject with non-zero callAsFunction in JSClassDefinition can have an arbitrary type, there is no need for it to inherit from InternalFunction. Only objects created with JSObjectMakeFunctionWithCallback() would be covered.
Comment 23 Yong Li 2010-06-01 12:50:19 PDT
(In reply to comment #22)
> > Yes, why not just do both checks?
> I'm not sure how that would help - a JSObject with non-zero callAsFunction in JSClassDefinition can have an arbitrary type, there is no need for it to inherit from InternalFunction. Only objects created with JSObjectMakeFunctionWithCallback() would be covered.

oops. I was lost. just wanted to make sure InternalFunction is not needed