1962205 - Overriding geolocation via webdriver lets two services active in parallel

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

(Context: https://bugzilla.mozilla.org/show_bug.cgi?id=1960651#c7)

I think we have a hidden webdriver-related bug we missed in bug 1951962.

The core problem is that we get two active backing services here: https://searchfox.org/mozilla-central/rev/8867630d67bfe76f6065df08d60381d45d73c439/dom/geolocation/Geolocation.cpp#1086-1092

  if (mService) {
    mService->AddLocator(this);
  }

  if (mServiceOverride) {
    mServiceOverride->AddLocator(this);
  }

Both are free to ping the locator, meaning even if watchPosition() is called after SetGeolocationServiceOverride(), a few seconds later the update from the "real" service will ping the locator again, resulting mishmash of signals from different services.

This has been hidden because of the timer, and removing the timer made it more apparent.

Sasha, care to take another look?

Comment 1

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1962205 - Move locators insted of running two geolocation services at a time.

Comment 2

[Alexandra Borovova [:Sasha]]

Yes, so I had a problem which this test illustrates: https://searchfox.org/mozilla-central/source/dom/geolocation/test/browser/browser_geolocation_override.js#245-358, that if we would add an override, reload the tab and then reset the override, a new service wouldn't have a locator. So the solution I came up with was to always have an original service as well, but as you said, it doesn't work with your patch. I found a different approach now: to move locators when we reset an override service. It seems to be working with your patch as well (all the tests pass).

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

(Not only it doesn't work with my patch, the webdriver users would also get confusing geolocation signals that they did not make)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c0be287c3ba
Move locators insted of running two geolocation services at a time. r=saschanaz

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0c0be287c3ba

Comment 6

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1962205 - Move locators insted of running two geolocation services at a time.

Original Revision: https://phabricator.services.mozilla.com/D246564

Comment 7

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: The geolocation API might sent the original coordinates togerher with the override
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: Low
• Explanation of risk level: It only affects test automation scenario
• String changes made/needed: None
• Is Android affected?: yes

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a302bdf7dc47