Closed Bug 911595 Opened 6 years ago Closed 6 years ago
Geolocation requests with timeout can be shutdown twice by callbacks spinning the event loop
If a getCurrentPosition request is made with a sufficiently small timeout value, and the position callback spins the event loop (for instance, by using alert()), it is possible for the timeout callback nsGeolocationRequest::Notify to be called while still inside the position callback. Conversely, if the error callback spins the event loop, it's possible for nsGeolocationRequest::SendLocation to be called while still inside the error callback. In both cases, since both nsGeolocationRequest::Notify and nsGeolocationRequest::SendLocation attempt to shutdown the request, we hit an assertion on debug builds and cause both callbacks (error and position) to be called on non-debug builds. This behavior is illustrated by the attached file.
One solution could be to call Shutdown() before triggering the callbacks.
Attachment #798318 - Flags: review?(josh)
Comment on attachment 798318 [details] [diff] [review] Avoid shutting down geolocation request twice. Review of attachment 798318 [details] [diff] [review]: ----------------------------------------------------------------- This is a good change. We should have tests for this - write a geolocation handler that sets a timer then spins the event loop, then inside the timer callback check that the error handler did not fire.
Attachment #798318 - Flags: review?(josh) → review+
New version with test.
The test needed to be disabled on Android and B2G because showModalDialog doesn't seem to be available for those. https://tbpl.mozilla.org/?tree=Try&rev=72eca1e40a0b
Comment on attachment 801636 [details] [diff] [review] Avoid shutting down geolocation request twice. Thanks!
Attachment #801636 - Flags: review?(josh) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cedb0b0bdd79 In the future, please respect the alphabetical ordering in the Android/B2G json files. I fixed them for you this time.
Assignee: nobody → ggp
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.