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

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Guilherme Gonçalves [:ggp]]

Attachment: 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.

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Avoid shutting down geolocation request twice.

One solution could be to call Shutdown() before triggering the
callbacks.

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 3

[Guilherme Gonçalves [:ggp]]

Attachment: Avoid shutting down geolocation request twice.

New version with test.

Comment 4

[Guilherme Gonçalves [:ggp]]

Attachment: 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

Comment 5

[Josh Matthews [:jdm]]

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

Thanks!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cedb0b0bdd79