search

paranext / paranext-core

Electron client, extension host, and C# library for Paranext
https://paranext.github.io/paranext-core/
MIT License
17 stars 2 forks source link

Extension UI can cause Platform to hang #1149

Open tombogle opened 2 months ago

tombogle commented 2 months ago

Describe the bug

An extension can display UI controls which cause Platform to hang. (Not sure of the specific details of what causes this, but it's consistent.)

To Reproduce

  1. Pull and build https://github.com/paranext/Platform.Bible-Psalms/tree/update-from-extension-template (specific commit here in case the branch gets deleted: https://github.com/paranext/Platform.Bible-Psalms/commit/a143206a5cbbbe91c1556102c234a36b9ca8fd7d)
  2. Run Platform with the Psalms: Layer by Layer extension
  3. On the menu, choose the command to open Psalms: Layer by Layer
  4. In the Psalms Layer by Layer webview (the one with the menus and checkboxes), either select a menu item in one of the menus or select one of the checkboxes.
  5. UI will completely freeze. No errors or messages of any sort are reported in Terminal or logged in the console window.

Expected behavior Ideally, of course, the selection would have the desired effect. But in any case, if the extension is not programed correctly, it would be really nice if the extension was unable to bring Platform to its knees. (MIght need to see what exactly is happening to see how feasible that would be.)

tjcouch-sil commented 1 month ago

Finally discovered that the Psalms LBL freeze was happening essentially in this situation (these conditions are met in one function component):

  1. useLocalizedStrings is running with a non-memoized array (so the component re-renders very frequently) - this is the real problem
  2. Some state on the component changes and causes its own rerender (or maybe the state that changed belonged to a parent; not sure) - this is where it looked like the problem was coming from

A week and a half of dev time down the tubes because the problem looked like it was coming from code that looks fine. I think am going to look for a good time soon to create some kind of low-cost check on these "must be stable" parameters that will log a warning if one changes more than x times a second and possibly throws if one changes more than y times a second or something along these lines. At least it will be some kind of dumb check rather than silent self-DoS. I don't really know of a better solution yet, but hopefully we can come up with something eventually.