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).
(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.
Disabled a test that checked Math.abs behavior in r60522 - it's currently different between JIT and Interpreter.
RegExps aren't callable in tot, we should probably just switch to CallType != CallTypeNone
Adding Leandro as this will be relevant to Media Streaming. See Bug 56459.
Created attachment 89070 [details] Patch
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.
Attachment 89070 [details] did not build on qt: Build output: http://queues.webkit.org/results/8390007
Created attachment 89127 [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. 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 on attachment 89127 [details] Patch r=me
Comment on attachment 89127 [details] Patch Clearing flags on attachment: 89127 Committed r83562: <http://trac.webkit.org/changeset/83562>
All reviewed patches have been landed. Closing bug.
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
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