SerenityOS
LibCore: Remove unnecessary or invalid write after child remove
Sometimes caused a segfault when typing text in the emoji search window. It also turned out to be unnecessary, since we would never want to write 0 to an address if it's null or after it has been set to 0 by remove_child.
Backtrace of "Insert Emoji" in the Text Editor
32.599 CrashReporter(53:63): --- Backtrace for thread #0 (TID 52) --- 32.599 CrashReporter(53:63): 0x0000000e7dd9d86f: [/usr/lib/libcore.so.serenity] Core::EventReceiver::remove_all_children() +0x2f (EventReceiver.h:138 => EventReceiver.cpp:103) 32.599 CrashReporter(53:63): 0x0000001370418e4f: [/usr/lib/libgui.so.serenity] GUI::EmojiInputDialog::update_displayed_emoji() [clone .localalias] +0x4f (EmojiInputDialog.cpp:164) 32.604 CrashReporter(53:63): 0x0000001370429f38: [/usr/lib/libgui.so.serenity] AK::Function<void ()>::operator()() const +0x48 (Function.h:115) 32.604 CrashReporter(53:63): 0x00000013704f85c7: [/usr/lib/libgui.so.serenity] non-virtual thunk to GUI::TextEditor::document_did_change(GUI::AllowCallback) [clone .localalias] +0x17 (TextEditor.h:304) 32.604 CrashReporter(53:63): 0x00000013704e0326: [/usr/lib/libgui.so.serenity] GUI::TextDocument::notify_did_change() [clone .localalias] +0x66 (TextDocument.cpp:173) 32.604 CrashReporter(53:63): 0x0000000ed4cc204d: [/usr/lib/libsyntax.so.serenity] Syntax::TextDocumentLine::insert(Syntax::Document&, unsigned long, unsigned int) +0x10d (Document.cpp:139) 32.604 CrashReporter(53:63): 0x00000013704e53e0: [/usr/lib/libgui.so.serenity] GUI::TextDocument::insert_at(Syntax::TextPosition const&, unsigned int, GUI::TextDocument::Client const*) [clone .localalias] +0x70 (TextDocument.cpp:1072) 32.608 CrashReporter(53:63): 0x00000013704e5620: [/usr/lib/libgui.so.serenity] GUI::TextDocument::insert_at(Syntax::TextPosition const&, AK::StringView, GUI::TextDocument::Client const*) +0x60 (TextDocument.cpp:1058) 32.608 CrashReporter(53:63): 0x00000013704e5d3e: [/usr/lib/libgui.so.serenity] GUI::InsertTextCommand::redo() +0x4e (TextDocument.cpp:785) 32.608 CrashReporter(53:63): 0x00000013704f87f4: [/usr/lib/libgui.so.serenity] GUI::TextEditor::insert_at_cursor_or_replace_selection(AK::StringView) [clone .localalias] +0x1a4 (TextDocument.h:162 => TextEditor.h:384 => TextEditor.cpp:1818) 32.608 CrashReporter(53:63): 0x00000013704f8dc7: [/usr/lib/libgui.so.serenity] GUI::TextEditor::add_code_point(unsigned int) [clone .part.0] +0xb7 (TextEditor.cpp:1397) 32.608 CrashReporter(53:63): 0x00000013704f9ca3: [/usr/lib/libgui.so.serenity] GUI::TextEditor::keydown_event(GUI::KeyEvent&) +0xe13 (TextEditor.cpp:1385 => TextEditor.cpp:1222) 32.611 CrashReporter(53:63): 0x00000013704db798: [/usr/lib/libgui.so.serenity] GUI::TextBox::keydown_event(GUI::KeyEvent&) +0x28 (TextBox.cpp:28) 32.611 CrashReporter(53:63): 0x0000001370531070: [/usr/lib/libgui.so.serenity] GUI::Widget::event(Core::Event&) +0x150 (Widget.cpp:287 => Widget.cpp:245) 32.611 CrashReporter(53:63): 0x0000000e7dd9da34: [/usr/lib/libcore.so.serenity] Core::EventReceiver::dispatch_event(Core::Event&, Core::EventReceiver*) +0x54 (EventReceiver.cpp:160) 32.616 CrashReporter(53:63): 0x000000137054a23a: [/usr/lib/libgui.so.serenity] GUI::Window::handle_key_event(GUI::KeyEvent&) [clone .localalias] +0x6a (Window.cpp:588) 32.616 CrashReporter(53:63): 0x000000137054a494: [/usr/lib/libgui.so.serenity] GUI::Window::event(Core::Event&) [clone .localalias] +0xc4 (Window.cpp:775) 32.616 CrashReporter(53:63): 0x0000000e7dd9da34: [/usr/lib/libcore.so.serenity] Core::EventReceiver::dispatch_event(Core::Event&, Core::EventReceiver*) +0x54 (EventReceiver.cpp:160) 32.616 CrashReporter(53:63): 0x0000000e7ddbce6b: [/usr/lib/libcore.so.serenity] Core::ThreadEventQueue::process() +0x2eb (ThreadEventQueue.cpp:111) 32.616 CrashReporter(53:63): 0x0000000e7dd9ae53: [/usr/lib/libcore.so.serenity] Core::EventLoopImplementationUnix::exec() +0x33 (EventLoopImplementationUnix.cpp:293 => EventLoopImplementationUnix.cpp:285) 32.619 CrashReporter(53:63): 0x0000000e7dd96c8c: [/usr/lib/libcore.so.serenity] Core::EventLoop::exec() +0x4c (EventLoop.cpp:88) 32.619 CrashReporter(53:63): 0x0000001370407efb: [/usr/lib/libgui.so.serenity] GUI::Dialog::exec() +0xab (Dialog.cpp:43) 32.619 CrashReporter(53:63): 0x00000013704fa275: [/usr/lib/libgui.so.serenity] GUI::TextEditor::insert_emoji() +0x125 (TextEditor.cpp:970) 32.619 CrashReporter(53:63): 0x000000137035367c: [/usr/lib/libgui.so.serenity] GUI::Action::activate(Core::EventReceiver*) [clone .localalias] +0x22c (Function.h:115) 32.619 CrashReporter(53:63): 0x00000013703b209d: [/usr/lib/libgui.so.serenity] GUI::ConnectionToWindowServer::menu_item_activated(int, unsigned int) +0x4d (ConnectionToWindowServer.cpp:302) 32.619 CrashReporter(53:63): 0x0000001370404b5e: [/usr/lib/libgui.so.serenity] WindowClientStub::handle(IPC::Message const&) +0x33e (WindowClientEndpoint.h:3082) 32.623 CrashReporter(53:63): 0x000000017009282c: [/usr/lib/libipc.so.serenity] IPC::ConnectionBase::handle_messages() +0xcc (Connection.cpp:88) 32.652 CrashReporter(53:63): 0x00000013703ba35d: [/usr/lib/libgui.so.serenity] AK::Function<void ()>::CallableWrapper<IPC::Connection<WindowClientEndpoint, WindowServerEndpoint>::Connection(IPC::Stub&, AK::NonnullOwnPtrCore::LocalSocket)::{lambda()#1}>::call() +0x4d (Connection.h:95 => Function.h:182) 32.652 CrashReporter(53:63): 0x0000000e7ddaf300: [/usr/lib/libcore.so.serenity] AK::Function<void ()>::CallableWrapperCore::LocalSocket::setup_notifier()::{lambda()#1}::call() +0x40 (Function.h:115) 32.654 CrashReporter(53:63): 0x0000000e7dda383c: [/usr/lib/libcore.so.serenity] Core::Notifier::event(Core::Event&) +0x5c (Function.h:115) 32.654 CrashReporter(53:63): 0x0000000e7dd9da34: [/usr/lib/libcore.so.serenity] Core::EventReceiver::dispatch_event(Core::Event&, Core::EventReceiver*) +0x54 (EventReceiver.cpp:160) 32.654 CrashReporter(53:63): 0x0000000e7ddbce6b: [/usr/lib/libcore.so.serenity] Core::ThreadEventQue ue::process() +0x2eb (ThreadEventQueue.cpp:111) 32.654 CrashReporter(53:63): 0x0000000e7dd9ae53: [/usr/lib/libcore.so.serenity] Core::EventLoopImplementationUnix::exec() +0x33 (EventLoopImplementationUnix.cpp:293 => EventLoopImplementationUnix.cpp:285) 32.654 CrashReporter(53:63): 0x0000000e7dd96c8c: [/usr/lib/libcore.so.serenity] Core::EventLoop::exec() +0x4c (EventLoop.cpp:88) 32.654 CrashReporter(53:63): 0x00000003e3d7fa6f: [/bin/TextEditor] serenity_main(Main::Arguments) +0xc4f (main.cpp:97) 32.658 CrashReporter(53:63): 0x00000003e3d68d37: [/bin/TextEditor] main +0x147 (Main.cpp:43) 32.658 CrashReporter(53:63): 0x00000003e3d68e94: [/bin/TextEditor] _entry +0x24 (crt0.cpp:48)
The patch looks sensible, but the fact that (what should be) a redundant operation appears to cause issues like this hints at a deeper problem (and we are just removing one of the symptoms here instead of the cause).
The patch looks sensible, but the fact that (what should be) a redundant operation appears to cause issues like this hints at a deeper problem (and we are just removing one of the symptoms here instead of the cause).
That may well be the case. I played around with the commit and it doesn't seem to break anything. I also find it interesting that only the emoji dialog caused problems. My guess is that the object from which we remove the parent is deleted by another thread or free call after the remove_child call and thus the segfault occurs when writing to the address of m_parent again. After all, we are removing a lot of emoji fields at the same time. But my guess could also be completely wrong.
I don't think there are any threads involved here. What is happening is that the child's ref count goes to zero after it is removed from its parent, because nothing else is holding a RefPtr to it. Thus it has been destroyed after the call to m_parent->remove_child(this). So referring to this->m_parent afterwards is no good because this is deleted.
So your change LGTM - but let's also add a comment for our future selves, since it's not super obvious at first glance:
void remove_from_parent()
{
if (m_parent)
m_parent->remove_child(*this);
// The call to `remove_child` may have deleted this object. Do not refer to `this` from this point forward.
}
Note that if we did need to keep the object alive to do more work after remove_child, we could do this:
void remove_from_parent()
{
NonnullRefPtr protector { *this };
if (m_parent)
m_parent->remove_child(*this);
// do more stuff with `this` here
}
but since setting m_parent = nullptr is a no-op (we do that already in remove_child), this isn't needed here.