keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
387 stars 107 forks source link

bug(ios): keyboard terminated by iOS due to use of too much memory #12216

Open jahorton opened 3 weeks ago

jahorton commented 3 weeks ago

Describe the bug

We have a host of Sentry events with the following title: "WatchdogTermination: The OS watchdog terminated your app, possibly because it overused RAM."

This is the way iOS enforces its memory-constraint rules on our system keyboard, which acts as a custom extension within iOS. According to React documentation, "48 MB or less." Other sources suggest "30 MB or less".

As noted by https://stackoverflow.com/a/45658368, there is a way to check for signals about running too close to the limit... but we don't always see those signals in the Sentry event reports.


Also, there's something... else I'm seeing in these reports that is a bit concerning. It also occurs in some of the "App hanging for at least 2000 ms." reports to do with iOS launch time constraints.

Here's one good candidate report. Go to the Breadcrumbs section, then click "View All" near its bottom. Do the same with any further linked reports in this description.

Of note for this report: "Loaded keyboard." appears 4 separate times in the breadcrumb logs. Timestamps:

Another report, this one having received a low-memory signal

This one has logs over a wider range of time, but similar pattern

I can't help but think about how we occasionally see new "instances" of the keyboard appear in the debug menu of desktop Safari when connected to a device + app for debugging the keyboard - where an old instance... technically still exists, but is no longer connected. Is the same underlying behavior happening on these devices?

... in which case, a question: should this happen, why are the old instances still "alive" in a sense? The natural assumption - there's something that prevents them from being garbage-collected. Since they can't be GC'd, we get a memory leak - and once grown too far, that would certainly be a reasonable cause to kill the app-extension.

If true, my next thought - we make use of the "Notification center" or similar for within-app notifications. Granted, the app notifications themselves shouldn't exist within the keyboard... but if there is some kind of notification that is listened for, in either direction - from the keyboard to Manager or vice-versa - something like that, if not disconnected, could easily be the site of lingering strong-references preserving the old instances.

Reproduce the bug

No response

Expected behavior

No response

Related issues

No response

Keyman apps

Keyman version

No response

Operating system

No response

Device

No response

Target application

No response

Browser

No response

Keyboard name

No response

Keyboard version

No response

Language name

No response

Additional context

No response

jahorton commented 3 weeks ago

... in which case, a question: should this happen, why are the old instances still "alive" in a sense? The natural assumption - there's something that prevents them from being garbage-collected. Since they can't be GC'd, we get a memory leak - and once grown too far, that would certainly be a reasonable cause to kill the app-extension.

If true, my next thought - we make use of the "Notification center" or similar for within-app notifications. Granted, the app notifications themselves shouldn't exist within the keyboard... but if there is some kind of notification that is listened for, in either direction - from the keyboard to Manager or vice-versa - something like that, if not disconnected, could easily be the site of lingering strong-references preserving the old instances.

Here's a prime example of what may be sustaining the old copies:

https://github.com/keymanapp/keyman/blob/65b3c75898ab5c5d52d415982564c18f5313666c/ios/engine/KMEI/KeymanEngine/Classes/Keyboard/KeymanWebViewController.swift#L70-L83

Note line 76 - we add an observer to the default notification center that maintains a reference to the KeymanWebViewController instance.

Later in the file:

https://github.com/keymanapp/keyman/blob/65b3c75898ab5c5d52d415982564c18f5313666c/ios/engine/KMEI/KeymanEngine/Classes/Keyboard/KeymanWebViewController.swift#L156-L159

While Swift does offer a few removeObserver variants... we use none of them.

https://developer.apple.com/documentation/foundation/notificationcenter/1413994-removeobserver

jahorton commented 3 weeks ago

FWIW, there are other parts in the code that similarly set up "observers" while not removing them later.

An iOS-project-wide search turns up 45 results in 16 files for addObserver... but only 5 results in 5 files for removeObserver.