Bug 29099 - Geolocation does not correctly handle Infinity for PositionOptions properties.
Summary: Geolocation does not correctly handle Infinity for PositionOptions properties.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-09 11:06 PDT by Steve Block
Modified: 2009-09-18 02:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch 1 for bug 29099 (16.86 KB, patch)
2009-09-09 11:19 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 2 for bug 29099 (15.64 KB, patch)
2009-09-09 17:15 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 3 for bug 29099 (17.17 KB, patch)
2009-09-10 04:10 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-09-09 11:06:30 PDT
Infinity is a valid value for the Geolocation PositionOptions.timeout and PositionOptions.maximumAge properties. Currently, Infinity is converted to int32 for these properties when the PositionOptions object is read. Instead, Infinity should be handled as a special value.
Comment 1 Steve Block 2009-09-09 11:19:38 PDT
Created attachment 39283 [details]
Patch 1 for bug 29099

Fixes bug and updates LayoutTest.
Comment 2 Eric Seidel (no email) 2009-09-09 15:03:06 PDT
Comment on attachment 39283 [details]
Patch 1 for bug 29099

It's a little strange that this is the first use of Inf.  Makes me wonder if it should be exported.  Obviously it must already be in a header somewhere.

In general this change looks fine.  I just find the Inf thing a bit strange.  Isn't there a posix function for detecting infitiy in a double?  Only positive infinity?  What about isinfinite(foo) instead?

I think r- for the strange Inf export and no discussion of handling negative infinity or not.

We should probably say "positive infinity" instead of "infinity" in the comments.
Comment 3 Steve Block 2009-09-09 16:10:09 PDT
> Isn't there a posix function for detecting infitiy in a double?
Yes, but I thought it better to use the JSC abstraction for Infinity, rather than resorting to platform-specifics. Inf is assigned in JavaScriptCore/runtime/JSCell.cpp, using different values dependent upon the platform. To safely test for infinity using platform-specifics, I'd have to repeat this logic.

>  Only positive infinity?
Yes, we're only interested in positive infinity. All negative values are converted to 0 for these parameters anyway.
 
> What about isinfinite(foo) instead?
isinf() tests for (x == +Inf || x == -Inf), but I'm not sure it's as safe for use across multiple platforms.
Comment 4 Eric Seidel (no email) 2009-09-09 16:28:49 PDT
wtf/MathExtras.h provides isinf(), so it seems we provide it for all platforms which don't support it natively.
Comment 5 Eric Seidel (no email) 2009-09-09 16:29:31 PDT
Using Inf could be totally fine, I don't know.  You'd have to ask a current JSC hacker.  Try olliej, mjs, ggaren or gbarra in #webkit.
Comment 6 Steve Block 2009-09-09 17:15:33 PDT
Created attachment 39315 [details]
Patch 2 for bug 29099

I checked with olliej and it's safe to use (isinf(x) && x > 0).

Have updated the patch to use this method, and fixed comments to use 'positive infinity'.
Comment 7 Eric Seidel (no email) 2009-09-09 17:18:28 PDT
Comment on attachment 39315 [details]
Patch 2 for bug 29099

So would:
 109         if (!(isinf(timeoutNumber) && (timeoutNumber > 0))) {
if (isfinite(timeoutNumber))

get you the same effect?  negativeInfinity will be ignored, which is maybe the wrong behavior?

Also, shouldn't we be testing negative infinity in the test cases?  Do the tests test any negative numbers?
Comment 8 Steve Block 2009-09-10 04:10:07 PDT
Created attachment 39339 [details]
Patch 3 for bug 29099

> > >  Only positive infinity?
> > Yes, we're only interested in positive infinity.
> (From update of attachment 39315 [details])
> So would:
>  109         if (!(isinf(timeoutNumber) && (timeoutNumber > 0))) {
> if (isfinite(timeoutNumber))
> get you the same effect?
No. The special case I want to handle (for both timeout and maximumAge) is +Inf. I'm not interested in -Inf. These values represent time limits, where a value of +Inf means 'no time limit applied'. All other values are converted to int32 and then negative values are set to 0. (This behaviour was chosen to follow the behaviour of window.setTimeout. See Bug 27254.)

The logic we need is ...

// Special case for +Inf - do nothing
if (timeout != +Inf)
  // Convert to Int32 and set negative values to 0

if (maximumAge == +Inf)
  // Special case for +Inf - clear maximum age
else
  // Convert to Int32 and set negative values to 0


Using ...

if (isfinite(timeout))

will cause a value of -Inf to skip the if, which is the wrong behaviour. It's true that I could use ...

if (isinfinite(timeout) || timeout < 0)

but I thought it better to match the structure of the logic to that used in the if statement for maximumAge.

> Also, shouldn't we be testing negative infinity in the test cases?
Given that the change adds a new code path only for +Inf, I thought testing +Inf was sufficient. I've added tests for -Inf too.
Comment 9 Steve Block 2009-09-10 04:12:51 PDT
> if (isinfinite(timeout) || timeout < 0)
oops, i meant ...
if (isfinite(timeout) || timeout < 0)
Comment 10 Steve Block 2009-09-11 02:56:30 PDT
> So are -Inf and +Inf always supposed to have the same behavior?
No, -Inf and +Inf are not supposed to have the same behaviour.

> I thought negative numbers were disallowed for someof these, no?
No, the correct behaviour is to accept (ie not throw an exception for) any JS type passed for these PositionOptions properties. We then convert to a number using the standard JS conversions.

The logic we follow is that I described previously. If the value is +Inf, we take the special-case action. Otherwise, we convert to int32 and then set negative values to zero, following the pattern of window.setTimeout.
Comment 11 Andrei Popescu 2009-09-11 03:20:58 PDT
Hi Eric,

(In reply to comment #10)
> > So are -Inf and +Inf always supposed to have the same behavior?

This is all covered by the spec. Here is the relevant section:

http://dev.w3.org/geo/api/spec-source.html#get-current-position

So, in brief, any negative number, including -Inf, will be converted to 0.



> > I thought negative numbers were disallowed for someof these, no?

Again, this in the spec: any numbers are allowed. The spec says what the implementation must do with the numbers given to it. -Inf will be converted to 0.

So this patch makes the implementation compliant with the spec.

Thanks,
Andrei
Comment 12 Eric Seidel (no email) 2009-09-15 23:25:23 PDT
Comment on attachment 39339 [details]
Patch 3 for bug 29099

Won't this break Chromium's builder?  This needs v8 changes too, no?  I mean, I'm OK approving as-is, but darin or dglazkov would probably yell at me. ;)
Comment 13 Andrei Popescu 2009-09-16 03:37:08 PDT
(In reply to comment #12)
> (From update of attachment 39339 [details])
> Won't this break Chromium's builder?  This needs v8 changes too, no?  I mean,
> I'm OK approving as-is, but darin or dglazkov would probably yell at me. ;)

(Steve's on vacation so I will try to cover for him while he's gone)

This change will not break chromium because, AFAIK, Chromium doesn't yet build Geolocation at all. The V8 bindings are missing. We have them written and they are in our queue of things to upstream.

Dimitri is CCed to this bug so can probably confirm. And, anyway, if this change somehow manages to break Chromium, then he should yell at us, not at you :)
Comment 14 Dimitri Glazkov (Google) 2009-09-16 07:20:28 PDT
Shouldn't break Chromium build. If it does, you'll hear from me :)
Comment 15 Dimitri Glazkov (Google) 2009-09-16 07:21:20 PDT
Comment on attachment 39339 [details]
Patch 3 for bug 29099

rs=me, based on Eric's feedback.
Comment 16 WebKit Commit Bot 2009-09-18 02:35:32 PDT
Comment on attachment 39339 [details]
Patch 3 for bug 29099

Clearing flags on attachment: 39339

Committed r48503: <http://trac.webkit.org/changeset/48503>
Comment 17 WebKit Commit Bot 2009-09-18 02:35:48 PDT
All reviewed patches have been landed.  Closing bug.