Bug 27255 - Geolocation incorrectly calls error callback in case of exception in success callback
Summary: Geolocation incorrectly calls error callback in case of exception in success ...
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: Nobody
URL:
Keywords:
Depends on: 28264 29027
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-14 04:46 PDT by Steve Block
Modified: 2009-09-09 09:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.12 KB, patch)
2009-07-27 03:56 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch for bug 27255 (4.40 KB, patch)
2009-07-27 04:15 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 3 for bug 27255 (8.36 KB, patch)
2009-09-09 03:20 PDT, Steve Block
darin: review+
Details | Formatted Diff | Diff
Patch 4 for bug 27255 (9.46 KB, patch)
2009-09-09 08:34 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-07-14 04:46:13 PDT
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.
Comment 1 Steve Block 2009-07-27 03:56:50 PDT
Created attachment 33531 [details]
Patch
Comment 2 Andrei Popescu 2009-07-27 04:05:09 PDT
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
Comment 3 Steve Block 2009-07-27 04:15:41 PDT
Created attachment 33532 [details]
Patch for bug 27255

Fixed ChangeLog to add bug URL and brief explanation of changes.
Comment 4 Greg Bolsinga 2009-07-31 15:13:06 PDT
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?
Comment 5 Andrei Popescu 2009-08-04 05:11:24 PDT
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
Comment 6 Greg Bolsinga 2009-08-04 09:53:05 PDT
(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.
Comment 7 Eric Seidel (no email) 2009-08-06 18:19:42 PDT
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.
Comment 8 Eric Seidel (no email) 2009-08-06 18:20:44 PDT
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!
Comment 9 Steve Block 2009-08-07 04:53:46 PDT
> 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?
Comment 10 Eric Seidel (no email) 2009-08-11 09:51:12 PDT
Comment on attachment 33532 [details]
Patch for bug 27255

This needs a layout test.  Otherwise it looks fine.
Comment 11 Steve Block 2009-08-11 10:02:28 PDT
> 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?
Comment 12 Steve Block 2009-09-09 03:20:20 PDT
Created attachment 39257 [details]
Patch 3 for bug 27255

Adds a LayoutTest for the fix.
Comment 13 Darin Adler 2009-09-09 07:50:44 PDT
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.
Comment 14 Steve Block 2009-09-09 08:34:37 PDT
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?
Comment 15 Adam Barth 2009-09-09 08:49:01 PDT
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.
Comment 16 Adam Barth 2009-09-09 08:50:54 PDT
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.
Comment 17 Eric Seidel (no email) 2009-09-09 09:27:40 PDT
Comment on attachment 39270 [details]
Patch 4 for bug 27255

Clearing flags on attachment: 39270

Committed r48210: <http://trac.webkit.org/changeset/48210>
Comment 18 Eric Seidel (no email) 2009-09-09 09:27:44 PDT
All reviewed patches have been landed.  Closing bug.