REGRESSION (r59811): Geolocation callbacks cannot be created

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Scott Menor]

navigator.geolocation.watchPosition(positionCallback)

fails and returns 

Error: TYPE_MISMATCH_ERR: DOM Exception 17

[Alexey Proskuryakov]

See also: bug 39454.

[Alexey Proskuryakov]

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

[Steve Block]

Is this with client-based geolocation, or the original geolocation implementation, or both?

[Alexey Proskuryakov]

This is an issue in JavaScriptCore bindings, so it's with both. I have a fix, but I need to make tests work first.

[Alexey Proskuryakov]

Created attachment 57391 [details]
proposed fix

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Attachment 57391 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2654017

[Alexey Proskuryakov]

I guess touching didn't work. Not sure how the build can be fixed without manual intervention.

[Darin Adler]

Comment on attachment 57391 [details]
proposed fix

> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        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.

> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        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.

[Steve Block]

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

[Steve Block]

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

[Yong Li]

why cannot the callbacks be InternalFunction?

[Steve Block]

> 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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

Created attachment 57561 [details]
proposed fix

Removed empty ChangeLog entries, re-worded a FIXME.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Attachment 57561 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2784056

[Steve Block]

> > 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.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/60488>.

[Alexey Proskuryakov]

> 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.

[Yong Li]

(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)) {

[Alexey Proskuryakov]

> 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.

[Yong Li]

(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