binaryninja-api icon indicating copy to clipboard operation
binaryninja-api copied to clipboard

Find Text runs on main thread

Open CouleeApps opened this issue 2 years ago • 4 comments

Version and Platform (required):

  • Binary Ninja Version: 3.0.3319-dev
  • OS: macOS
  • OS Version: 12.0.1

Bug Description: When searching for text in the UI, the search runs on the main thread and waits for IL generation on the main thread. This causes the UI to lock up for larger files where IL generation can take many seconds per function.

Steps To Reproduce: Please provide all steps required to reproduce the behavior:

  1. Open any large binary. I'm using ntdll
  2. Search for text in HLIL, I used "and rsp"
  3. Observe monster lag spikes

Expected Behavior: I expected the UI to continue being responsive while searching was in progress. The UI isn't modal so I did not expect it to hang the main thread waiting for results.

Additional Information: MainWindow::findAll looks like it directly calls View::findAllText, which should probably be done instead in a background thread.

CouleeApps avatar Apr 05 '22 20:04 CouleeApps

I had a look at this and I agree that we should call the findAll functions on a background thread. However, in my testing, the UI does not freeze while the search is underway -- potentially thanks to the call to QCoreApplication::processEvents(); inside of the progress function. I know this is not a reliable way to do it, so I support the change. Just trying to see if there is a possible case where the UI freezes entirely?

xusheng6 avatar May 04 '22 10:05 xusheng6

The progress function only keeps the ui responsive if it's called often enough, and for larger bndbs this is not the case. Eg ntdll can spend multiple seconds generating the IL for functions, so multiple seconds pass before the progress function is called and the ui can update.

CouleeApps avatar May 04 '22 17:05 CouleeApps

Reverting this due to crashes, re-targeted for 3.2.

psifertex avatar May 27 '22 15:05 psifertex

Good job GitHub. Reverting a commit with a message mentioning the issue closes it. 🤦‍♂️

psifertex avatar May 27 '22 16:05 psifertex