Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't crash during a slow (>= 60 seconds) startup on X11 #225

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

vedgy
Copy link
Contributor

@vedgy vedgy commented Mar 3, 2021

See the added source code comments and the commit message for details.

The patch I used for benchmarking (stupid GitHub won't let me attach a patch => had to zip it): 0001-Benchmark-QCoreApplication-processEvents-workaround.patch.zip.

@selmf
Copy link
Member

selmf commented Mar 5, 2021

Is this a good idea? If I understand your testing process correctly, the crash you want to fix here occurs in a highly artificial scenario (forced interruption/delay of main event loop) and is only needed for some very specific testing scenarios. So the chances of this happening during a normal startup are very slim and the usefulness of enforcing process events are doubtful.

If it really is needed, it should be contained to the specific scenario it was written for, i.e. debug builds only and possibly X11 only too if it is irrelevant for the other systems.

@vedgy
Copy link
Contributor Author

vedgy commented Mar 5, 2021

I have been hit by this crash many times over the last month while debugging Reader and Library, and not in highly artificial scenarios. Though of course end users won't encounter it unless their computers are extremely slow.

The actual scenarios:

  1. Sometimes a developer uses a debugger to step through code that runs at the application's startup. If the Q(Core|)Application::exec() call in main() is thus delayed by more than a minute, the debugging session is interrupted by the crash.
  2. I have recently rebuilt my system qt5-base package by applying diffs from https://github.com/qutebrowser/qt-debug-pkgbuild/tree/master/qt5-base to be able to step into Qt sources while debugging. Turns out that the already slow YACReader debug session startup becomes much slower when Qt debug symbols are available - now it always takes more than 60 seconds on my system. Thus attaching to an already running Reader process became the only alternative to this workaround. The attaching requires changing system ptrace_scope permissions, is less convenient and does not allow to debug the application startup.

Restricting the workaround to Debug builds and/or X11 has a potential to introduce inconsistencies between platforms and build modes. Some bugs may manifest themselves with the event processing, but not without, or the other way around. This can easily become a maintenance burden and a source of Release-only or platform-specific bugs. I think it is best to apply and try it out on all supported platforms. Hopefully it won't cause any issues and can remain unconditional. If you don't want to risk introducing a bug into the upcoming YACReader release, this workaround can wait until after the new version is released. Then we will have an entire release cycle to notice regressions.

@vedgy vedgy force-pushed the fix-crash-at-slow-startup branch from 7e55f17 to c62dfd0 Compare March 5, 2021 15:35
@vedgy
Copy link
Contributor Author

vedgy commented Mar 5, 2021

Just found the upstream bug report: QTBUG-58709. Linked it with the Stack Overflow workaround and replaced other links in this PR's commit message with the Qt bug ID.

@selmf
Copy link
Member

selmf commented Mar 5, 2021

I am still undecided on the best way to proceed here and I will likely need to do some more reflection and some tests of my own before reaching a decision.

As a general rule, if there is a QTBUG that we need to implement a workaround for we always include a comment with the bug id so we can easily identify and check it at a later point in development. So please add

// Workaround for QTBUG-58709

or something along the lines to the relevant sections so we can find it when grepping for QTBUG :)

The corresponding upstream bug - QTBUG-58709 - was reported in 2017 and
was de-prioritized from P2 to P3 in 2020.

A quick benchmark shows that the added QCoreApplication::processEvents()
call takes the same time in Debug and Release builds of YACReader and
YACReaderLibrary - from 0.5 to 0.6 milliseconds. 0.6 milliseconds is not
a noticeable startup slowdown, especially considering that the event
loop does useful work: processes events, which would have to be handled
eventually anyway.

Don't restrict the workaround to Linux/X11 or Debug mode: no need to
introduce a potential behavior difference between supported platforms
or build types.

The same workaround can be applied to YACReaderLibraryServer, but I
don't use the server and cannot test it.
@vedgy vedgy force-pushed the fix-crash-at-slow-startup branch from c62dfd0 to 3b8db55 Compare March 5, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants