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.

Sign up for GitHub

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

Jump to bottom

Measurement: Fix macos crashes caused by unhandled exceptions reaching qt #16906

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Measurement: Fix macos crashes caused by unhandled exceptions reaching qt #16906

merged 1 commit into from
Oct 3, 2024

Conversation

hyarion
Copy link
Contributor

@hyarion hyarion commented Sep 29, 2024 •
edited
Loading

As described in #16905, macos crashes when exceptions are reaching qt.

The crash this PR fixes is described this comment: #16888 (comment)

The reason why FreeCAD crashes is:

  1. OCCT throws an exception here
  2. The exception is not caught in QuickMeasure::processSelection()

This PR adds a catch for OCCT exceptions as well as a generic for unknown exceptions.

Note, this doesn't fix the bug, but it fixes the crash.

@github-actions github-actions bot added the Mod: Measurement Regarding Measurment functionality label Sep 29, 2024
@hyarion hyarion changed the title Fix macos crashes when unhandled exceptions reaches qt Measurement: Fix macos crashes caused by unhandled exceptions reaching qt Sep 29, 2024
@maxwxyz maxwxyz added this to the 1.0 milestone Sep 29, 2024
@maxwxyz maxwxyz added the Needs backport Needs backport to 1.0 label Sep 29, 2024
@luzpaz luzpaz mentioned this pull request Sep 29, 2024
Closed
2 tasks
@PaddleStroke
Copy link
Contributor

So I was looking into this before seeing this PR.
In Measurement.cpp there're also 2 places where there's no catch(...) block. My understanding on this topic is a bit limited, so can you please advise if they should also be added?

@hyarion
Copy link
Contributor Author

hyarion commented Sep 30, 2024

@PaddleStroke
We must be really careful and catch all exceptions so reaches qt code.
In linux exceptions passes through down to GuiApplication::notify without a problem, but on macOS (arm64) it crashes.

More information an links to a Qt bug on the topic can be found here

What this means for us is that all places where a linux user might get a error message Unhandled {some exception} caught in GUIApplication::notify. must be handled earlier. AIRCAP wrote a good comment about this here.

For this bug, the callstack looked follows (click to expand)

At the top of the stack is where the thread was killed due to unhandled exception (it never goes through qt on macos:arm64, on linux it does and is handled)

  • libsystem_kernel.dylib__pthread_kill 8`
  • libsystem_pthread.dylibpthread_kill 288`
  • libsystem_c.dylibabort 180`
  • libc abi.dylibabort_message 132`
  • libc abi.dylibdemangling_terminate_handler() 348`
  • libobjc.A.dylib_objc_terminate() 160`
  • libc abi.dylibstd::__terminate(void (*)()) 16`
  • libc abi.dylib__cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) 88`
  • libc abi.dylib__cxa_throw 308`

The exception was thrown here:

  • Measure.soTopoDS::Edge(S=0x000000016fdfb770) at TopoDS.lxx:71:3`
  • Measure.soMeasure::Measurement::length(this=0x000000012cdbff40) const at Measurement.cpp:361:43`
  • MeasureGui.soMeasureGui::QuickMeasure::printResult(this=0x000000012cdbf840) at QuickMeasure.cpp:208:42`
  • MeasureGui.soMeasureGui::QuickMeasure::tryMeasureSelection(this=0x000000012cdbf840) at QuickMeasure.cpp:101:5`
  • MeasureGui.soMeasureGui::QuickMeasure::processSelection(this=0x000000012cdbf840) at QuickMeasure.cpp:81:13`
  • MeasureGui.soQtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MeasureGui::QuickMeasure::*)()>::call(f=(MeasureGui.soMeasureGui::QuickMeasure::processSelection() at QuickMeasure.cpp:77), o=0x000000012cdbf840, arg=0x000000016fdfbe48) at qobjectdefs_impl.h:152:13
  • MeasureGui.sovoid QtPrivate::FunctionPointer<void (MeasureGui::QuickMeasure::*)()>::call<QtPrivate::List<>, void>(f=(MeasureGui.soMeasureGui::QuickMeasure::processSelection() at QuickMeasure.cpp:77), o=0x000000012cdbf840, arg=0x000000016fdfbe48) at qobjectdefs_impl.h:185:13

MeasureGui was invoked using the slot here:

  • MeasureGui.soQtPrivate::QSlotObject<void (MeasureGui::QuickMeasure::*)(), QtPrivate::List<>, void>::impl(which=1, this_=0x000000012cdc0f00, r=0x000000012cdbf840, a=0x000000016fdfbe48, ret=0x0000000000000000) at qobjectdefs_impl.h:418:17`
  • libQt5Core.5.15.8.dylib___lldb_unnamed_symbol16968 820`

The slot was called by a timedout QTimer:

  • libQt5Core.5.15.8.dylibQTimer::timeout(QTimer::QPrivateSignal) 52`
  • libQt5Core.5.15.8.dylibQObject::event(QEvent*) 104`

The event was sent by QApplication's notify:

  • libQt5Widgets.5.15.8.dylibQApplicationPrivate::notify_helper(QObject*, QEvent*) 232`
  • libQt5Widgets.5.15.8.dylibQApplication::notify(QObject*, QEvent*) 472`

Here is Gui::GuiApplication::notify where we are trying to catch the event, but it doesn't work as explained in the Qt ticket

  • libFreeCADGui.dylibGui::GUIApplication::notify(this=0x000000016fdfe2b8, receiver=0x000000012cdc0e60, event=0x000000016fdfc430) at GuiApplication.cpp:83:34`
  • libQt5Core.5.15.8.dylibQCoreApplication::notifyInternal2(QObject*, QEvent*) 176`

The tail here isn't really that interesting, but I'll keep it for the record:

  • libQt5Core.5.15.8.dylibQTimerInfoList::activateTimers() 520`
  • libqcocoa.dylib___lldb_unnamed_symbol3667 20`
  • libqcocoa.dylib___lldb_unnamed_symbol3666 16`
  • CoreFoundationCFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION 28`
  • CoreFoundation__CFRunLoopDoSource0 176`
  • CoreFoundation__CFRunLoopDoSources0 244`
  • CoreFoundation__CFRunLoopRun 828`
  • CoreFoundationCFRunLoopRunSpecific 608`
  • HIToolboxRunCurrentEventLoopInMode 292`
  • HIToolboxReceiveNextEventCommon 648`
  • HIToolbox_BlockUntilNextEventMatchingListInModeWithFilter 76`
  • AppKit_DPSNextEvent 660`
  • AppKit-[NSApplication(NSEventRouting) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 700`
  • AppKit-[NSApplication run] 476`
  • libqcocoa.dylib___lldb_unnamed_symbol3677 968`
  • libQt5Core.5.15.8.dylibQEventLoop::exec(QFlagsQEventLoop::ProcessEventsFlag) 312`
  • libQt5Core.5.15.8.dylibQCoreApplication::exec() 120`
  • libFreeCADGui.dylib(anonymous namespace)::tryRunEventLoop(mainApp=0x000000016fdfe2b8) at Application.cpp:2207:5`
  • libFreeCADGui.dylib(anonymous namespace)::runEventLoop(mainApp=0x000000016fdfe2b8) at Application.cpp:2227:9`
  • libFreeCADGui.dylibGui::Application::runApplication() at Application.cpp:2304:5`
  • FreeCADmain(argc=3, argv=0x000000016fdfeef8) at MainGui.cpp:295:13`
  • dyldstart 2360`

As can be seen in the call stack, the point of entry was processSelection and since there already was a try-catch-structure there, I simply added:

  • a case for the base OCCT exception (Standard_Failore), and
  • a case for any other type of exception

In other words, I moved up the error handling from GuiApplication::notify to the last point before the exception would reach Qt which would cause a crash.

I haven't checked if there are more slots which could be called. If so those needs to be handled as well.

To answer your question,
I think it would be best to add exception handling in all slots, but that discussion would be better to have in this issue.

I looked at the two try-catch structures in measurement and it seems like they are utility functions, not slots, so it might not be needed.

@hyarion hyarion mentioned this pull request Sep 30, 2024
Closed
2 tasks
@chennes chennes merged commit 9913d6a into FreeCAD:main Oct 3, 2024
10 checks passed
@chennes
Copy link
Member

chennes commented Oct 3, 2024

Backported to 1.0

@hyarion hyarion mentioned this pull request Oct 14, 2024
Closed
2 tasks
@chennes chennes removed the Needs backport Needs backport to 1.0 label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Mod: Measurement Regarding Measurment functionality
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@hyarion @PaddleStroke @chennes @maxwxyz