Bug 40012

Summary: Callable objects created via JavaScriptCore API cannot be used as Geolocation callbacks
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Steve Block <steveblock>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, kbalazs, leandrogracia, oliver, ossy, sam, steveblock, webkit-ews, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 57770    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Alexey Proskuryakov 2010-06-01 11:24:32 PDT
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).
Comment 1 Gavin Barraclough 2010-06-01 15:32:35 PDT
(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.
Comment 2 Alexey Proskuryakov 2010-06-01 17:31:19 PDT
Disabled a test that checked Math.abs behavior in r60522 - it's currently different between JIT and Interpreter.
Comment 3 Oliver Hunt 2011-02-16 10:35:25 PST
RegExps aren't callable in tot, we should probably just switch to CallType != CallTypeNone
Comment 4 Steve Block 2011-03-24 04:06:59 PDT
Adding Leandro as this will be relevant to Media Streaming. See Bug 56459.
Comment 5 Steve Block 2011-04-11 13:37:58 PDT
Created attachment 89070 [details]
Patch
Comment 6 Oliver Hunt 2011-04-11 13:40:47 PDT
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.
Comment 7 Early Warning System Bot 2011-04-11 13:51:18 PDT
Attachment 89070 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8390007
Comment 8 Steve Block 2011-04-11 17:10:56 PDT
Created attachment 89127 [details]
Patch
Comment 9 Steve Block 2011-04-11 17:13:35 PDT
> 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.
Comment 10 Oliver Hunt 2011-04-11 17:15:06 PDT
Comment on attachment 89127 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2011-04-12 01:13:50 PDT
Comment on attachment 89127 [details]
Patch

Clearing flags on attachment: 89127

Committed r83562: <http://trac.webkit.org/changeset/83562>
Comment 12 WebKit Commit Bot 2011-04-12 01:13:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2011-04-12 02:11:03 PDT
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
Comment 14 Balazs Kelemen 2011-04-12 02:39:30 PDT
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