Bug 40012 – Callable objects created via JavaScriptCore API cannot be used as Geolocation callbacks

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Geolocation specification requires that only "Function objects" can be used for Geolocation callbacks. We currently require the callback to inherit from JSFunction, which is almost definitely incorrect, because this doesn't include functions created via API.

In JSC, there are two ways to check for being callable:
1) Check callData return value. That's the closest to just trying to call the object, but it includes RegExp objects, which we may or may not want to exclude;
2) Check that className is "Function". That seems to match the current WebIDL text best, but it doesn't cover objects created via API.

The reason for the spec requirement is reportedly to disallow objects with handleEvent property, and that makes sense, because the callbacks aren't events. But Geolocation shouldn't really be different from Database or other APIs that have callbacks. Web APIs don't check for callback argument types upfront (I actually suspect that a JS object can even become callable - or cease to be callable - at some point after creation, making upfront checks inadequate).

[Gavin Barraclough]

(In reply to comment #0)
> Geolocation specification requires that only "Function objects" can be used for Geolocation callbacks. We currently require the callback to inherit from JSFunction, which is almost definitely incorrect, because this doesn't include functions created via API.
> 
> In JSC, there are two ways to check for being callable:
> 1) Check callData return value. That's the closest to just trying to call the object, but it includes RegExp objects, which we may or may not want to exclude;
> 2) Check that className is "Function". That seems to match the current WebIDL text best, but it doesn't cover objects created via API.

I'd suggest the best check here would be to test that the value is callable, but is not a regexp - i.e.:
    ((value.getCallData() != CallTypeNone) && !value.inherits(&RegExpObject::info))
Generally speaking, the ECMA spec requires a 1-1 correlation between objects being callable and considered a function – regexps are one class where we follow other browsers in make an exception to this rule (not doing so breaks the web).

> The reason for the spec requirement is reportedly to disallow objects with handleEvent property, and that makes sense, because the callbacks aren't events. But Geolocation shouldn't really be different from Database or other APIs that have callbacks. Web APIs don't check for callback argument types upfront (I actually suspect that a JS object can even become callable - or cease to be callable - at some point after creation, making upfront checks inadequate).

Whether an object is or is not callable is not statically typed – it is determined by the result of a virtual function call.  The are no restrictions to ensure that for a given object the result is repeatable – but that said, I can't think of any example of an object where this would change.  The only classes I can think that may produce more than one result would be JSFunction (CallTypeJS or CallTypeHost) and API objects (CallTypeNone or CallTypeHost) - but in both cases this should be fixed from object creation.

[Alexey Proskuryakov]

Disabled a test that checked Math.abs behavior in r60522 - it's currently different between JIT and Interpreter.

[Oliver Hunt]

RegExps aren't callable in tot, we should probably just switch to CallType != CallTypeNone

[Steve Block]

Adding Leandro as this will be relevant to Media Streaming. See Bug 56459.

[Steve Block]

Created attachment 89070 [details]
Patch

[Oliver Hunt]

Comment on attachment 89070 [details]
Patch

Can we add an actual API callback as a test?  I'm not sure why Math.abs wouldn't work in the past but it should pass with the inherits check.  That means we're not actually testing the right thing in this case.

[Early Warning System Bot]

Attachment 89070 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8390007

[Steve Block]

Created attachment 89127 [details]
Patch

[Steve Block]

> Can we add an actual API callback as a test?  I'm not sure why Math.abs
> wouldn't work in the past but it should pass with the inherits check.  That
> means we're not actually testing the right thing in this case.
Yes, using Math.abs works for me without this fix. Alexey mentioned that the behaviour is different with JIT vs the Interpreter.

I've added a test using layoutTestController.setGeolocationPermission, which fails without this fix.

[Oliver Hunt]

Comment on attachment 89127 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 89127 [details]
Patch

Clearing flags on attachment: 89127

Committed r83562: <http://trac.webkit.org/changeset/83562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

This change made fast/dom/Geolocation/argument-types.html fail on the WK2 bot:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r83562%20(10596)/fast/dom/Geolocation/argument-types-pretty-diff.html

[Balazs Kelemen]

WebKit2 has no layoutTestController.setGeolocationPermission currently so we need
to skip this test since now it is using it.
SkipList update has been landed in http://trac.webkit.org/changeset/83571