SurfaceWidget signals should be disconnected in destructor

[Eeems]

@Eeems Looking beyond what we've ruled out, I found a serious issue in the signal connection lifecycle:

// In Surface constructor when !EPAPER
connect(this, &Surface::update, widget, &SurfaceWidget::update);
connect(widget, &SurfaceWidget::activeFocusChanged, this, &Surface::activeFocusChanged);

There's no corresponding disconnect() in the destructor. With Qt's signal/slot system, this creates a dangerous situation. Even though shared_ptrs prevent premature object destruction, the Qt signal connections can still cause crashes when:

1. The Surface is destroyed
2. The SurfaceWidget is still alive
3. The SurfaceWidget receives an event that causes it to emit activeFocusChanged
4. Qt tries to deliver this signal to the now-destroyed Surface object

This explains the intermittent crashes - they only happen when events trigger signals at specific times during object destruction.

To fix this, add in the destructor (when !EPAPER):

#ifndef EPAPER
if(component != nullptr) {
    auto widget = component->findChild<SurfaceWidget*>("Surface");
    if(widget != nullptr) {
        disconnect(this, &Surface::update, widget, &SurfaceWidget::update);
        disconnect(widget, &SurfaceWidget::activeFocusChanged, this, &Surface::activeFocusChanged);
    }
    component->deleteLater();
    component = nullptr;
}
#endif

This properly disconnects signals before the Surface is destroyed, preventing Qt from trying to deliver signals to a freed object.

---

🧬 Code Definitions Used

Relevant Code Snippets

1. applications/display-server/connection.cpp

void Connection::close(){
    if(!m_closed.test_and_set()){
        m_pingTimer.stop();
        m_notRespondingTimer.stop();
        for(auto& item : surfaces){
            item.second->removed();
        }
        emit finished();
    }
}

• Description: This method closes the connection and marks it as closed. It stops timers and calls the removed() method on all surfaces associated with the connection.
• Line Range: 187-196

2. applications/system-service/screenshot.cpp

void removed();

• Description: This function likely marks an object as removed, similar to the removed() method in the Surface class.
• Line Range: 46-46

3. applications/system-service/notification.cpp

void removed();

• Description: This function also likely marks an object as removed, similar to the removed() method in the Surface class.
• Line Range: 132-132

4. applications/system-service/wifiapi.cpp

void WifiAPI::update(){
    // ... (code omitted for brevity)
    if(m_state != state){
        setState(state);
    }
    // ... (code omitted for brevity)
}

• Description: This method updates the state of the WiFi API and emits signals based on changes. It may relate to the management of connections and surfaces.
• Line Range: 770-770

5. shared/liboxide/event_device.cpp

void event_device::close(){
        if(fd > 0){
            ::close(fd);
        }
        fd = 0;
    }

• Description: This method closes the event device by closing the file descriptor if it is valid.
• Line Range: 32-37

6. shared/libblight/connection.cpp

maybe_ackid_ptr_t Connection::repaint(
        surface_id_t identifier,
        int x,
        int y,
        unsigned int width,
        unsigned int height,
        WaveformMode waveform,
        unsigned int marker
    ){
        if(!identifier){
            errno = EINVAL;
            return {};
        }
        repaint_t repaint{
            {
                .x = x,
                .y = y,
                .width = width,
                .height = height,
                .waveform = waveform,
                .marker = marker,
                .identifier = identifier,
            }
        };
        auto ackid = send(MessageType::Repaint, (data_t)&repaint, sizeof(repaint));
        if(!ackid.has_value()){
            return {};
        }
        return ackid;
    }

• Description: This method handles repaint requests for surfaces. It checks if the identifier is valid and sends a repaint message.
• Line Range: 213-221

7. shared/libblight/connection.h

repaint(
            surface_id_t identifier,
            int x,
            int y,
            unsigned int width,
            unsigned int height,
            WaveformMode waveform = WaveformMode::HighQualityGrayscale,
            unsigned int marker = 0
        )

• Description: This function signature indicates a method for repainting a surface, which may be relevant for ensuring surfaces are properly managed.
• Line Range: 128-136

8. applications/display-server/dbusinterface.cpp

void DbusInterface::repaint(QString identifier, QDBusMessage message){
    auto connection = getConnection(message);
    if(!connection->has("system")){
        sendErrorReply(QDBusError::AccessDenied, "Must be system connection");
        return;
    }
    auto surface = getSurface(identifier);
    if(surface == nullptr){
        sendErrorReply(QDBusError::BadAddress, "Surface not found");
        return;
    }
    surface->repaint();
}

• Description: This method handles repaint requests via D-Bus, checking for valid connections and surfaces before proceeding.
• Line Range: 223-235

These snippets may provide context or functionality related to the management of surfaces and their destruction, which is relevant to the user's comment about ensuring proper guarding against surfaces being destroyed.

Originally posted by @coderabbitai[bot] in #397 (comment)