search

mozilla-mobile / focus-ios

⚠️ Firefox Focus (iOS) has moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-ios
Mozilla Public License 2.0
1.26k stars 263 forks source link

[3.8 Crasher] OverlayView #503

Closed Sdaswani closed 7 years ago

Sdaswani commented 7 years ago

2cOkHemcLCTd9SkpD8C-k.xccrashpoint.zip

Sdaswani commented 7 years ago

You can also find this in XCode -> Organizer -> Focus -> Crashes

Krishna commented 7 years ago

I started taking a look into this issue.

The crash logs indicate that the problem manifests in the OverlayView setAttributedButtonTitle(phrase: button:) method.

So I pulled out the relevant code into a Playground (attached) with some minor refactoring to make it easier to push in different test data.

firefox-focus-overlay-bug-2.playground.zip

I was working with the hypothesis that maybe some edge case data is causing that method to have problems.

But so far I have not been able to reproduce the problem in the playground (or running the Firefox focus code base in a simulator). Perhaps someone else can give it a try with any test data they have?

I also looked at the code that invokes this method in the OverlayView class. There is some dispatch queue stuff going on relating to the Pasteboard that I am not too sure of.

I think the AutocompleteTextField should really be handling any paste events, and perhaps communicating that via its delegate (which would need to be modified) to the URLBar and the OverlayView. But maybe none of this is what's causing the crash, and I am missing some context as to why the code is the way it is.

Do we have any idea on how to recreate the crash reliably? I'll try and look more into this issue when I get a chance.

boek commented 7 years ago

Hey @Krishna, thank you for looking into this! I definitely agree with your hypothesis, though I haven't been able to reproduce the crash myself. I've been tossing around the idea that it could be an edge case with a different locale/language or some Unicode that is getting input that we're not accounting for. Definitely a puzzling one.

Regarding the dispatch queue around the pasteboard: That method does a blocking call when fetching the shared clipboard from iCloud. So we move it off of the main thread.

bkmunar commented 7 years ago

First, I tried testing in different locales to see if any sort of specific and unique characters were causing the crash in OverlayView. I tried Arabic and various other languages, putting in anything and everything in the native keyboard, but to no avail. I tried putting in super long strings as well.

Also, I realized this crash ONLY started occurring after the Swift 4 port, so maybe it had to do with the new changes in Swift 4 that related to string cutting/replacing that is handled in OverlayView? However, looking at the crash report... the number of crashes are going down overtime (refer to the bar graph in the picture)... either meaning people are somehow avoiding crashing their phone with this specific crasher and/or they're not using Focus anymore (which I think neither is the case).

Maybe this has been somehow fixed in the latest iOS version? This has been discussed with Jeff and we're wondering maybe if we just make the search attribute text function safer for the next release; we can monitor this specific crasher and see if it spikes up again since we don't have a specific STR to know whether we've killed this bug or not.

screen shot 2017-10-03 at 3 16 06 pm
Krishna commented 7 years ago

I opened a PR https://github.com/mozilla-mobile/focus-ios/pull/526 with some light refactoring + added a comment about the Pasteboard issue to the relevant bit in the code.

Not a fix unfortunately, but hopefully will make the issue easier to test, and for any replacement logic to easily be substituted in.