2023581 - Geolocation mStarting state becomes stale after UpdateAccuracy, causing StartDevice to skip AddGeolocationListener

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Florian Quèze [:florian]]

test_enableHighAccuracy.html is a permanent failure when run standalone (blocking bug 2010766). The root cause is a bug in nsGeolocationService::UpdateAccuracy: When UpdateAccuracy in the content process sends SetGeolocationHigherAccuracy to the parent, the parent removes and re-adds its geolocation listener with the new accuracy. However, UpdateAccuracy does not update mStarting to reflect this change. This leaves mStarting stale, and a subsequent StartDevice call may incorrectly skip sending AddGeolocationListener when the requested accuracy matches the stale mStarting value.

Concrete scenario:

1. watchPosition(enableHighAccuracy: true) calls StartDevice, setting mStarting = Some(true) and sending AddGeolocationListener(true) to the parent.
2. clearWatch calls UpdateAccuracy, which sends SetGeolocationHigherAccuracy(false) to the parent (parent switches to low accuracy), but mStarting remains Some(true).
3. A later getCurrentPosition(enableHighAccuracy: true) calls StartDevice, which sees mStarting = Some(true) matching highAccuracyRequested = true and skips, never sending AddGeolocationListener(true).
4. The request is served by a stale position update from the parent's old low-accuracy listener, and no high-accuracy notification is ever fired.

The fix updates mStarting in UpdateAccuracy when sending SetGeolocationHigherAccuracy, so it accurately reflects the accuracy state communicated to the parent.

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 2023581 - Update mStarting in UpdateAccuracy to prevent stale accuracy state. r=#dom-core

Comment 2

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

What does "standalone" mean here? Running the test separately, like ./mach test (test path) instead of being part of a bigger test set?

Comment 3

[Florian Quèze [:florian]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #2)

What does "standalone" mean here? Running the test separately, like ./mach test (test path) instead of being part of a bigger test set?

Yes. Locally it means run the test alone rather than as part of an entire folder of test. On CI it will mean restarting the browser between every test.

Comment 4

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

I think I regressed this...

Comment 5

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1960651

Comment 6

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

The test doesn't really wait for the geolocation requests to finish, it only waits for setHighAccuracy. That works only when it's called only once for each call...

1. The first watchPosition calls StartDevice from the content process (sets mStarting), and that leads to UpdateAccuracy and StartDevice in the parent process.
2. The test waits for that (by waiting for setHighAccuracy), and calls clearWatch. That calls UpdateAccuracy from the content process via Shutdown(), and then ultimately in the parent process. Ultimately. Well.
3. Now the second watchPosition happens in the test, and the test waits for another UpdateAccuracy.
4. What's the expectation now? It waits for the clearance of the first watchPosition, instead of the second one's start. The clearance resets the high accuracy flag, so the test still passes.
5. Then the test proceeds to call clearWatch for the second watchPosition. But it didn't even have the chance to actually call StartDevice yet. When StartDevice is called, it does nothing as it's already cleared. This second clearWatch does not call UpdateAccuracy as that happens only when high accuracy is requested, and the second one did not request it.
6. Then the test calls getCurrentPosition, which calls StartDevice, and the test waits for UpdateAccuracy. So what's happening here is that nothing cleared mStarting yet and thus it stays (which makes sense, the device hasn't responded yet), and that's the value set by step 1 here. This clearing happens only when the device actually responded at all - via nsGeolocationService::Update. That hasn't happened yet, because the test did not waited for that.
7. So we already unset high accuracy flag via UpdateAccuracy-from-clearWatch, but mStarting stays with the high accuracy flag set, so StartDevice thinks it's safe to skip the parent process.
8. Then where's the third UpdateAccuracy coming from? That's from nsGeolocationRequest::SendLocation's Shutdown call. Which again unsets the flag. Boom, the test is angry now.

6 - 8 doesn't matter much in real situation - even if it calls UpdateAccuracy again, nothing makes sure the resulting location data is with the requested accuracy, because the parent process may already be sending a location. At least not with the current architecture.

I recommend to:

1. Wait for at least one location signal, to make sure we wait until the device actually started.
2. Query the accuracy flag instead of depending on promiseOneMessage("setHighAccuracy").

Does this make sense, Florian? I can also take it if you prefer so.

Comment 7

[Florian Quèze [:florian]]

Feel free to take it. I know nothing about this code/test, other than that it perma fails currently when running the test alone.

Comment 8

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Attachment: Bug 2023581 - Replace promiseOneMessage with sendQuery for accuracy check r=#dom-core

It's not guaranteed that we set accuracy only once, it's rather guaranteed that we set twice at least - set it true and then set it back to false. The test assumes it's set exactly once per each API call, so we need to stop assuming that and rather query the status instead.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/27440112ddf5
https://hg.mozilla.org/integration/autoland/rev/62f6f5384404
Replace promiseOneMessage with sendQuery for accuracy check r=dom-core,smaug

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/62f6f5384404