mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.23k stars 2.94k forks source link

JS alerts can still cause crashes (e.g Scan QR Code App shortcut) #13974

Closed gutley closed 1 year ago

gutley commented 1 year ago

Steps to reproduce

• Visit a website that displays one of the JS alert types, (alert, message or prompt) and display such an alert without closing it)

e.g - https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_alert2

• Whilst displaying the alert, go back to the phone's home screen and long-press the Firefox icon. • Choose "Scan QR Code"

See screen recording

Expected behavior

Firefox should not crash

Actual behavior

Firefox does crash

Device & build information

Notes

This is a duplicate of https://github.com/mozilla-mobile/firefox-ios/issues/9541 as once again the alert's completion block is not being called in all cases before JSPromptAlertController is deinited.

Annoyingly, I provided a PR to fix this permanently over a year ago, but it was decided not to use it. A similar fix still works with the latest code, but I feel submitting a new PR will be equally unwanted.

Attachments:

https://user-images.githubusercontent.com/2040690/232418012-5c15a95b-3c1d-473a-8184-85a78e28bc58.mov

┆Issue is synchronized with this Jira Task

data-sync-user commented 1 year ago

➤ Yoana Rios Diaz commented:

Andrei Bodea there were still one case missing for the JSAlert crash do you mind checking the steps for this one but also some test for the previous bugs related to this crash because there was a refactoring involved to solve this issue Thank you

data-sync-user commented 1 year ago

➤ Andrei Bodea commented:

Hello, Yoana Rios Diazplease note that I could not reproduce this issue, and nothing related to all the linked issues on the latest v9000 with iPhone 13 Pro (15.7.1).

data-sync-user commented 1 year ago

➤ Andrei Bodea commented:

Do you think we can close this one? Yoana Rios Diaz

data-sync-user commented 1 year ago

➤ Yoana Rios Diaz commented:

Yes Andrei Bodea we can close it and monitor Sentry

data-sync-user commented 1 year ago

➤ Andrei Bodea commented:

Closing based on the comments below, thank you Yoana Rios Diaz