Bug 22847 - Geolocation PositionOptions cannot be an arbitrary object
Summary: Geolocation PositionOptions cannot be an arbitrary object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-13 15:01 PST by Greg Bolsinga
Modified: 2008-12-15 13:12 PST (History)
1 user (show)

See Also:


Attachments
Patch implementing the fix (8.00 KB, patch)
2008-12-13 15:27 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
Updated patch (11.96 KB, patch)
2008-12-15 12:08 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2008-12-13 15:01:11 PST
Geolocation PositionOptions cannot be an arbitrary object

From webkit-dev:
Date: Fri, 12 Dec 2008 12:50:35 -0800 (PST)
From: Aurelian Maga aurelianmaga at yahoo com
To: webkit-dev@lists.webkit.org

Hello,

I'm trying the geolocation api (gtk implementation with geoclue) and in a javascript I'm creating a postionoptions object like this:

var options = {enableHighAccuracy:true,timeout:30000};

and pass this to getCurrentPosition.  The GeolocationServiceGtk::startUpdating(PositionOptions* options) is being called but the options object is null.

Could someone point me in  the right direction to fix this?

Thank you,
Relu
Comment 1 Greg Bolsinga 2008-12-13 15:27:49 PST
Created attachment 26006 [details]
Patch implementing the fix

This is the patch from Sam.
Comment 2 Darin Adler 2008-12-14 18:18:12 PST
Comment on attachment 26006 [details]
Patch implementing the fix

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 39282)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +2008-12-13  Greg Bolsinga  <bolsinga@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=22847
> +        
> +        PositionOptions can be any aribitrary object.
> +
> +        * DerivedSources.make:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * bindings/js/JSGeolocationCustom.cpp:
> +        (WebCore::createPositionOptions):
> +        (WebCore::JSGeolocation::getCurrentPosition):
> +        (WebCore::JSGeolocation::watchPosition):
> +        * page/PositionOptions.idl:

I'd like to see slightly more detailed comments explaining the changes.

Also, arbitrary is misspelled here.

> +    // FIXME: Appropriately deal with an exception being thrown.

Is this really something we have to put off? Can't you make a test case that throws an exception, and decide what we want to do in that case?

> +    unsigned timeout = timeoutValue->toInt32(exec);

Should be toUInt32 if you're storing this in an unsigned.

And are you sure we want the toUInt32 rules? This do modulo arithmetic. Another option is to use toNumber and then range check and convert to an integer. You should make test cases that are outside the 32-bit integer range and see what they do.

> Index: WebCore/page/PositionOptions.idl
> ===================================================================
> --- WebCore/page/PositionOptions.idl	(revision 39281)
> +++ WebCore/page/PositionOptions.idl	(working copy)
> @@ -25,9 +25,8 @@
>  
>  module core {
>  
> -    interface [
> -        GenerateConstructor
> -    ] PositionOptions {
> +    // This interface generates no binding for JavaScript.
> +    interface PositionOptions {
>          attribute boolean enableHighAccuracy;
>          attribute unsigned long timeout;
>      };

I think we should eliminate the IDL file entirely.

I see you taking things out of DerivedSources.make and the Xcode project. What about the build files for other platforms? Won't this break the build?

I'm going to say review- because I don't think it's OK to break all the other platforms. Please consider my other comments too.
Comment 3 Sam Weinig 2008-12-15 12:08:30 PST
Created attachment 26027 [details]
Updated patch

Greg, this is an updated version of the patch which hopefully addresses Darin's comments.  It would great if you could test this with some malformed PositionOptions to make sure all is well.

Example of malformed PositionOptions include:

// Exception being thrown
var o = { get timeout() { throw 1; } }

// Overflow (note UINT_MAX doesn't exist in JS, so you will have to fill it in yourself)
var o = { timeout: UINT_MAX + 1; }
Comment 4 Darin Adler 2008-12-15 12:34:55 PST
Comment on attachment 26027 [details]
Updated patch

r=me
Comment 5 Sam Weinig 2008-12-15 13:12:06 PST
Fixed in r39311.