Bug 27255 – Geolocation incorrectly calls error callback in case of exception in success callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Currently, if an exception occurs when invoking the user-supplied success callback, the error callback (if supplied) is invoked. See Geolocation::sendPositionToOneShots() and Geolocation::sendPositionToWatchers().

The error callback is intended to report errors in the Geolocation location acquisition process itself, not errors in the user's script. See http://www.w3.org/TR/geolocation-API/#geolocation. For this reason, the error callback should not be invoked in the case of exceptions in the success callback.

[Steve Block]

Created attachment 33531 [details]
Patch

[Andrei Popescu]

Your ChangeLog doesn't look right: you need the bug URL and some brief explanation of the changes.  Please see the "ChangeLog files" section at

http://webkit.org/coding/contributing.html

Andrei

[Steve Block]

Created attachment 33532 [details]
Patch for bug 27255

Fixed ChangeLog to add bug URL and brief explanation of changes.

[Greg Bolsinga]

Oh, the reason I did that was this message:

http://lists.w3.org/Archives/Public/public-geolocation/2008Oct/0088.html

I'm open to this suggestion, however. Andrei?

[Andrei Popescu]

Hi,

The spec currently says:

"UNKNOWN_ERROR (numeric value 0)
The location acquisition process failed due to an error not covered by the definition of any other error code in this interface."

http://dev.w3.org/geo/api/spec-source.html#unknown_error

So UNKNOWN denotes an error in acquiring a position (it is a catch-all value for any situation not covered by the other error codes). If the UA is executing the success callback, then it means that the position was successfully acquired so, as far as the UA is concerned, the job is done. 

It can, of course, happen that the success callback throws an exception but I think this is a problem with the user code and not the Geolocation API implementation. As such, error callback should not be invoked. Instead, I think an error event should be raised and window.onerror should be invoked (if defined) or the error should be logged to the JS console.

Andrei

[Greg Bolsinga]

(In reply to comment #5)

> It can, of course, happen that the success callback throws an exception but I
> think this is a problem with the user code and not the Geolocation API
> implementation. As such, error callback should not be invoked. Instead, I think
> an error event should be raised and window.onerror should be invoked (if
> defined) or the error should be logged to the JS console.

Great. I think the code already will report the exception in that way, so this patch implements what you describe above. I'm not a reviewer, however. I'm going to move the bug to "NEW", in hope it gets reviewed.

[Eric Seidel (no email)]

Comment on attachment 33532 [details]
Patch for bug 27255

OK.  That information/justification from the spec really should go in the ChangeLog.  Please update it when landing.

[Eric Seidel (no email)]

Comment on attachment 33532 [details]
Patch for bug 27255

Oh.  You can't land your own yet, can you.  Please post one final patch with the justification information from the spec.

Something like "these are marked [NoInterfaceObject] in the spec:" with the links you provided

Thanks!

[Steve Block]

> Something like "these are marked [NoInterfaceObject] in the spec:" with the
> links you provided
This appears to refer to Bug 27250. To which does bug does your previous comment refer? Do you have any comments for this bug?

[Eric Seidel (no email)]

Comment on attachment 33532 [details]
Patch for bug 27255

This needs a layout test.  Otherwise it looks fine.

[Steve Block]

> This needs a layout test.
Adding a layout test for this change requires a mock implementation of GeolocationService such that we can guarantee that the success callback is called.  I plan to add such a mock implementation (and hooks for DumpRenderTree) as part of Bug 27716, but it might take a while to get the whole framework in place. Would it be OK to submit this patch now, but leave the bug open to add the layout tests once the mock GeolocationService is in place?

[Steve Block]

Created attachment 39257 [details]
Patch 3 for bug 27255

Adds a LayoutTest for the fix.

[Darin Adler]

Comment on attachment 39257 [details]
Patch 3 for bug 27255

>      if (exec->hadException()) {
>          reportCurrentException(exec);
> -        raisedException = true;
>      }

Just noticed that braces should go now that this is only one line.

> -    virtual void handleEvent(Geoposition*, bool& raisedException);
> +    virtual void handleEvent(Geoposition*);

This should be private, not public.

> +        virtual void handleEvent(Geoposition* position) = 0;

The argument name "position" should be omitted.

[Steve Block]

Created attachment 39270 [details]
Patch 4 for bug 27255

> Just noticed that braces should go now that this is only one line.
Fixed

This should be private, not public.
Fixed, and also in JSCustomPositionErrorCallback.h

> The argument name "position" should be omitted.
Fixed

Could you commit on my behalf please - I don't have committer rights?

[Adam Barth]

Comment on attachment 39270 [details]
Patch 4 for bug 27255

Once Darin reviews this updated patch, a committer can mark it commit-queue+ and it will be committed by the bot.

[Adam Barth]

Comment on attachment 39270 [details]
Patch 4 for bug 27255

Actually, in this case, I can just forward Darin's r+ of your previous patch.

[Eric Seidel (no email)]

Comment on attachment 39270 [details]
Patch 4 for bug 27255

Clearing flags on attachment: 39270

Committed r48210: <http://trac.webkit.org/changeset/48210>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.