Closed Bug 1589618 Opened 5 years ago Closed 4 years ago

Move the implementation of the region fetch to NetworkGeolocationProvider to have it close to the wifi scanning code.

Categories

(Firefox :: Search, enhancement, P1)

Product:
Firefox
Component:
Search
Type:
enhancement
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 75
Iteration:
75.2 - Feb 24 - Mar 8
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: mikedeboer, Assigned: daleharvey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

Bug 1589618 - Move the implementation of the region fetch to NetworkGeolocationProvider to have it close to the wifi scanning code. r?Standard8!,garvan!
47 bytes, text/x-phabricator-request
Details | Review

Pending Privacy Policy update signoff.

Iteration: 71.4 - Oct 14 - 20 → 72.1 - Oct 21 - Nov 3
Iteration: 72.1 - Oct 21 - Nov 3 → 72.3 - Nov 18 - Dec 1
Attachment #9112577 - Attachment description: Bug 1589618 - Ensure that essential browser features in the URLBar are not blocked by the region fetch. r?Standard8! → Bug 1589618 - Ensure that essential browser features in the URLBar are not blocked by the region fetch and ensure engines are reloaded after. r?Standard8!

So this now has all the required sign-offs and is pending reviews.

Attachment #9112577 - Attachment description: Bug 1589618 - Ensure that essential browser features in the URLBar are not blocked by the region fetch and ensure engines are reloaded after. r?Standard8! → Bug 1589618 - Ensure that essential browser features in the URLBar are not blocked by the region fetch and ensure engines are reloaded properly after. r?Standard8!
Attachment #9103535 - Attachment description: Bug 1589618 - Since we don't have a synchronous init flow of the Search Service anymore, keeping track of separate region fetch timeout isn't necessary anymore. r?Standard8!,bmiroglio! → Bug 1589618 - Since we don't have a synchronous init flow of the Search Service anymore, keeping track of separate region fetch timeout isn't necessary. r?Standard8!,bmiroglio!

Dale, I just submitted the latest of my patches that implement the changes Mark requested and it's blowing test suite(s) up like it's nothing. Well, it's not as bad as my earlier try pushes, but still... It'd be great if you could take a look at the code and perhaps point out something I missed? Thanks!!

Flags: needinfo?(dharvey)

So as mentioned in search, these tests are failing due to the change in behaviour of maybeReloadEngines

https://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_geodefaults.js#18 should be |Services.search.init(true)|

However the main issue is that in the calls to |asyncReInit({ waitForRegionFetch: true })| the search service will explicity wait for the region fetch to complete, but then also schedule maybeReloadEngines to happen at some point later, at this point later the tests arent expecting us to run and we may be in a funny state.

We could solve this by passing through |awaitRegionFetch| to |_maybeReloadEngines|, this should fix most of the test usage, if you can avoid changing this behaviour we "may" want to look fixing it as part of a larger refactor

Flags: needinfo?(dharvey)
Assignee: mdeboer → dharvey
Depends on: 1612206
Depends on: 1613627
Attachment #9103533 - Attachment description: Bug 1589618 - Rename all 'geo.wifi' related preferences to match the 'geo.provider.' convention and move default values to all.js. r?garvan! → Bug 1613627 - Rename all 'geo.wifi' related preferences to match the 'geo.provider.' convention and move default values to all.js. r?garvan!
Attachment #9103534 - Attachment description: Bug 1589618 - Rename 'WifiGeo' to the more generic and appropriate 'NetworkGeo' inside the module. r?garvan! → Bug 1613627 - Rename 'WifiGeo' to the more generic and appropriate 'NetworkGeo' inside the module. r?garvan!

Comment on attachment 9103534 [details]
Bug 1613627 - Rename 'WifiGeo' to the more generic and appropriate 'NetworkGeo' inside the module. r?garvan!

Revision D50210 was moved to bug 1613627. Setting attachment 9103534 [details] to obsolete.

Attachment #9103534 - Attachment is obsolete: true

Comment on attachment 9103533 [details]
Bug 1613627 - Rename all 'geo.wifi' related preferences to match the 'geo.provider.' convention and move default values to all.js. r?garvan!

Revision D50209 was moved to bug 1613627. Setting attachment 9103533 [details] to obsolete.

Attachment #9103533 - Attachment is obsolete: true
Attachment #9112577 - Attachment is obsolete: true
Attachment #9103535 - Attachment is obsolete: true

Ok so I have all the tests passing except one memory leak that is introduced by this patch, (try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=31093c62cd87b25e358b3799ae7d2240d5b84c20)

Mike, the memory leak is caused by the |Services.tm.dispatchToMainThread(() => this.sendLocationRequest(null));| in the startup() @ https://hg.mozilla.org/try/file/4a71e1592a5594f713131ff6678497113ad250f7/dom/system/NetworkGeolocationProvider.jsm#l310

Investigating why this codepath is being run since I wouldnt expect it to via dom/tests/mochitest/geolocation/test_manyCurrent which is where I see the memory leak, however either way its probably indicating something is wrong, one of the change I made from your version to mine was moving the dispatch to before |if (this.started)| because otherwise there was a bunch of tests waiting for geolocation requests that never got a response.

However given that change and this memory leak, any change you could give me a quick rundown about what the plan and implications of the changes were here so I have a bit more context for the fixes. I can probably make changes that gets the tests passing but want to make sure they are the right ones. Cheers

Flags: needinfo?(mdeboer)

Mike answered on Matrix (cheerS)

Flags: needinfo?(mdeboer)
Attachment #9103537 - Attachment is obsolete: true
Attachment #9103536 - Attachment is obsolete: true
Iteration: 72.3 - Nov 18 - Dec 1 → 75.2 - Feb 24 - Mar 8

We should probably re-title this bug for what we're actually landing as well.

No longer blocks: search-optimization
Group: mozilla-employee-confidential
CC list accessible: false
Flags: needinfo?(dharvey)
Not accessible to reporter
Summary: Implement adding public WiFi data to MLS queries behind a pref → Move the implementation of the region fetch to NetworkGeolocationProvider to have it close to the wifi scanning code.

ok found this, a timeout pref got rewritten with the wrong value, it was only failing on asan tests on taskcluster as it was a timeout that would only fail when running particularly slowly

Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eeda4bdcd130
Move the implementation of the region fetch to NetworkGeolocationProvider to have it close to the wifi scanning code. r=Standard8,garvan

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox75: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
Regressions: 1619587
Depends on: 1654542
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: