Differential D4542

Fix crash with Memory Viewer enabled
ClosedPublic
Actions

Authored by antonanikin on Feb 10 2017, 9:05 AM.

Details

Reviewers
apol
Group Reviewers
KDevelop
Commits
R32:2b60043d7b2a: Fix crash with Memory Viewer enabled
Summary

Steps to reproduce (tested on kubuntu 16.04):

  1. Set some breakpoint
  2. Run debug
  3. Close Memory View
  4. KDevelop crashed

Or simply run debug without breakpoint and get "hang-up" of KDevelop at the end of debug session.

The problem of crash was in incorrect memory operations - MemoryViewerWidget class creates MemoryView objects and sets them QObject* parent to self. This leads to situation when connected MemoryViewerWidget::slotChildDestroyed method is called after finish of MemoryViewerWidget destructor.

Test Plan

Tested with with master branch

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
antonanikin updated this revision to Diff 11146.Feb 10 2017, 9:05 AM
antonanikin retitled this revision from to Fix crash with Memory Viewer enabled.
antonanikin updated this object.
antonanikin edited the test plan for this revision. (Show Details)
antonanikin added a reviewer: KDevelop.
antonanikin added a subscriber: kdevelop-devel.
antonanikin updated this object.Feb 10 2017, 9:08 AM
apol added a subscriber: apol.Feb 10 2017, 11:15 AM

What makes you think it's going to fix your crash? You're deleting parented objects

antonanikin updated this revision to Diff 11167.Feb 10 2017, 12:31 PM
  • Simplify code
antonanikin added a comment.Feb 10 2017, 12:36 PM
In D4542#84911, @apol wrote:

You're deleting parented objects

What's wrong with such deletion?

Anyway new version completely solve the original problem by removing this useless list :)

apol accepted this revision.Feb 10 2017, 11:00 PM
apol added a reviewer: apol.

:) much better!

This revision is now accepted and ready to land.Feb 10 2017, 11:00 PM
Closed by commit R32:2b60043d7b2a: Fix crash with Memory Viewer enabled (authored by antonanikin). · Explain WhyFeb 11 2017, 1:08 AM
This revision was automatically updated to reflect the committed changes.
volden added a subscriber: volden.Feb 11 2017, 1:45 PM

Aren't we leaking memory views now?

How about removing the signal and just deleting all memoryViews in MemoryViewWidgets dtor?

antonanikin added a comment.Feb 11 2017, 1:58 PM
In D4542#85367, @volden wrote:

Aren't we leaking memory views now?
How about removing the signal and just deleting all memoryViews in MemoryViewWidgets dtor?

Hi, Morten. No, we haven't memleaks after the patch because of using QObject parent-child mechanism:

MemoryView* widget = new MemoryView(this); // line 458

The fixed crash was caused by wrong combination of automatic (QObject mechanism) and manual (slotChildDestroyed()) approaches for memory management.

volden added a comment.Feb 11 2017, 2:25 PM
In D4542#85373, @antonanikin wrote:
In D4542#85367, @volden wrote:

Aren't we leaking memory views now?
How about removing the signal and just deleting all memoryViews in MemoryViewWidgets dtor?

Hi, Morten. No, we haven't memleaks after the patch because of using QObject parent-child mechanism:

MemoryView* widget = new MemoryView(this); // line 458

The fixed crash was caused by wrong combination of automatic (QObject mechanism) and manual (slotChildDestroyed()) approaches for memory management.

Ah, yes. You're quite right. No need for the list. :-)

Revision Contents
Changeset List

Diff 11167

View Options

debuggers/gdb/memviewdlg.h

Loading...
View Options

debuggers/gdb/memviewdlg.cpp

Loading...