1926196 - Clicking Yes/No on the `Let Windows apps access your location` window after visiting a page in the same tab and then requesting location permission again will cause a crash

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: crash_geo_step2.gif

Found in

• 133.0a1 (2024-10-21)

Affected versions

• 133.0a1 (2024-10-21)
• 132.0
• 131.0.2
• 128.4.0esr

Tested platforms

• Affected platforms: Windows 11 24H2
• Unaffected platforms: Windows 10, macOS 12, Ubuntu

Preconditions

• Windows 11 24H2 (26100.2152) / (26100.2033)
• Geolocation was not used before - make a new Windows user profile and follow the steps

Steps to reproduce

1. Open https://developer.mozilla.org/en-US/docs/Web/API/Geolocation_API/Using_the_Geolocation_API#result
2. Click Show my location button and then Click on Allow button.
3. Open a new webapge in the tab where the geolocation is requested.
4. Click the back arrow.
5. Click Show my location button and click Yes on the Windows location request.

Expected result

• No crash.

Actual result

• Firefox crashes. Attaching a screen recording.

Regression range

• Most likely started with bug 1900225.

Additional notes

• Attached a screen recording.
• This has the same crash signature as bug 1926201 but David asked to file separate issues bug1917201#comment33

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:handyman, since you are the author of the regressor, bug 1900225, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 3

[Donal Meehan [:dmeehan]]

:handyman any updates on this? Wondering how the investigation is going and if we can expect anymore fixes?

Comment 4

[David Parks [:handyman]]

See bug 1926201 comment 4

Comment 5

[Pascal Chevrel:pascalc]

David, is that an S4 given the volume? Should it be assigned? Thanks

Comment 6

[David Parks [:handyman]]

Agreed. I'm looking at it now.

Comment 7

[David Parks [:handyman]]

Attachment: Bug 1926196: Use HashSet for wifi listeners r=#necko-reviewers!

This avoids invalidating iterators when mutating the collection during iteration.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a35a5a6fbcce
Use HashSet for wifi listeners r=necko-reviewers,valentin

Comment 9

[Cristina Horotan [:chorotan]]

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

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:handyman, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[David Parks [:handyman]]

As a test for this shows (as suggested in the review), HashSet isn't a fix for this. This is, presumably, an example of the slightly changed crash that comes with the recent patch. The problem is that, ModIterator ("A hash table iterator that permits modification, removal and rekeying.") only applies to the remove method on ModIterator itself. The remove method on the HashTable/HashSet object will still cause an iterator invalidation. I'm going to have to iterate over a copy of the collection and check for removal at each iteration.

Comment 12

[David Parks [:handyman]]

Attachment: Bug 1926196: Iterate over a copy of WifiListenerHolder HashSet to allow mutation r=#necko-reviewers!

HashSet::ModIterator::Remove does not invalidate iterators but
HashSet::Remove does. We need to preserve iteration on removal from the
collection itself, so we either need a collection that guarantees that
(like std::set) or a copy. We use an array copy here, for optimal asymptotic
performance.

Includes a test that both OnChange and OnError allow listener removal.

Comment 13

[David Parks [:handyman]]

Attachment: Bug 1926196: Iterate over a copy of WifiListenerData to allow mutation r=#necko-reviewers!

To avoid iterators being invalidated during iteration, this needs a collection
that allows mutation during iteration (like std::set) or a copy. This uses
a copy to get the best asymptotic performance.

Includes a test that both OnChange and OnError allow listener removal.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cab543d891e7
Iterate over a copy of WifiListenerData to allow mutation r=necko-reviewers,kershaw

Comment 15

[Atila Butkovits]

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

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

I can no longer reproduce the issue using Firefox 135.0a1 (2024-12-05) on Windows 11x64 and aarch64 by following the steps from comment 0. I also can no longer reproduce bug 1926201 so I'm closing bug 1926201 as a duplicate of this issue.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

No crashes on Nightly since the second patch landed. Are these ready for Beta and ESR128 approval requests? They graft cleanly to both.

Comment 19

[David Parks [:handyman]]

Comment on attachment 9441498 [details]
Bug 1926196: Iterate over a copy of WifiListenerData to allow mutation r=#necko-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Crashes on Windows in some cases when geolocation is requested. Bug 1935354 is listed as also needed because it fixes the build for MinGW, but I don't know if MinGW builds are important to beta/ESR?
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1935354
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is a different collections data structure. It is only visible to users when it causes things not to crash. It's also small and self contained.
• String changes made/needed: N/A
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 20

[David Parks [:handyman]]

Comment on attachment 9441498 [details]
Bug 1926196: Iterate over a copy of WifiListenerData to allow mutation r=#necko-reviewers!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This has a fairly high volume of crashes on Windows.
• User impact if declined: Crashes on Windows in some cases when geolocation is requested. Bug 1935354 is listed as also needed because it fixes the build for MinGW, but I don't know if MinGW builds are important to beta/ESR?
• Fix Landed on Version: 135
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is a different collections data structure. It is only visible to users when it causes things not to crash. It's also small and self contained.

Comment 21

[Pascal Chevrel:pascalc]

David, cab543d891e7 does not graft cleanly to the beta branch. Could you rebase your patch? (probably the same for ESR). Also, please ask for uplift in Bug 1935354 (and make sure the patch applies to the branch). Thanks!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1926196: Use HashSet for wifi listeners

This avoids invalidating iterators when mutating the collection during iteration.

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1926196: Iterate over a copy of WifiListenerData to allow mutation

To avoid iterators being invalidated during iteration, this needs a collection
that allows mutation during iteration (like std::set) or a copy. This uses
a copy to get the best asymptotic performance.

Includes a test that both OnChange and OnError allow listener removal.

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1926196: Use HashSet for wifi listeners

This avoids invalidating iterators when mutating the collection during iteration.

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1926196: Iterate over a copy of WifiListenerData to allow mutation

To avoid iterators being invalidated during iteration, this needs a collection
that allows mutation during iteration (like std::set) or a copy. This uses
a copy to get the best asymptotic performance.

Includes a test that both OnChange and OnError allow listener removal.

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

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/bb174f18bd68
https://hg.mozilla.org/releases/mozilla-beta/rev/6c9a1f6dc459

Comment 27

[David Parks [:handyman]]

Thanks Ryan for picking that up while I was on PTO.

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/f9207caf1bbb
https://hg.mozilla.org/releases/mozilla-esr128/rev/aa34db7b015a

Comment 29

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Verified fixed using Firefox 128.6.0esr (20241216192234) from comment 28 and Firefox 134.0b11 (20241216180229) from comment 26 (treeherder builds) on Windows 11 aarch64 by following the steps from comment 0. I also verified steps from bug 1926201 and that issue no longer reproduces.