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

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ggp, Assigned: ggp)

Details

Attachments

(2 files, 2 obsolete files)

Attached file test file
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.
Attachment #798318 - Attachment is obsolete: true
Attachment #800822 - Flags: review?(josh)
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
Attachment #800822 - Attachment is obsolete: true
Attachment #800822 - Flags: review?(josh)
Attachment #801636 - Flags: review?(josh)
Comment on attachment 801636 [details] [diff] [review]
Avoid shutting down geolocation request twice.

Thanks!
Attachment #801636 - Flags: review?(josh) → review+
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cedb0b0bdd79
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.