Geolocation requests with timeout can be shutdown twice by callbacks spinning the event loop

RESOLVED FIXED in mozilla26

Status

()

Core
Geolocation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ggp, Assigned: ggp)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 798317 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 798318 [details] [diff] [review]
Avoid shutting down geolocation request twice.

One solution could be to call Shutdown() before triggering the
callbacks.
Attachment #798318 - Flags: review?(josh)

Comment 2

4 years ago
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+
(Reporter)

Comment 3

4 years ago
Created attachment 800822 [details] [diff] [review]
Avoid shutting down geolocation request twice.

New version with test.
Attachment #798318 - Attachment is obsolete: true
Attachment #800822 - Flags: review?(josh)
(Reporter)

Comment 4

4 years ago
Created attachment 801636 [details] [diff] [review]
Avoid shutting down geolocation request twice.

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 5

4 years ago
Comment on attachment 801636 [details] [diff] [review]
Avoid shutting down geolocation request twice.

Thanks!
Attachment #801636 - Flags: review?(josh) → review+
(Reporter)

Updated

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.