Workaround the bug found by ASan, which can be seen on FreeBSD CI.
ClosedPublic

Authored by arrowd on Jan 23 2019, 6:33 AM.

Details

Summary

Currently many tests fail (at least on FreeBSD) with ASan turned on. This is what happening:

  • test_toolviewtoolbar creates an action, a ToolDocument and a View in the init() function.
  • ToolViewAction registers itself as event filter for the created View.
  • When cleanup is called, the controller gets deleted. This causes removal of its children (QObject::deleteChildren()), included the mentioned View.
  • Somewhere later during destruction, some QEvent arrives and gets passed to ToolViewAction::eventFilter(). This function accesses ->widget() method of m_dock->view(), but that View is already deleted, which causes ASan error.

Note that adding removeEventFilter() to ToolViewAction::~ToolViewAction() doesn't solve the problem - the event arrival happens before ToolViewAction destruction. I take it, there is some ownage confusion, but I wasn't able to figure it out.

So, I just flipped the condition in the if statement and it happens that it is also false, so the second condition doesn't get executed anymore.

I'd be happy to fix this proper way, but need some clues on where to dig.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arrowd created this revision.Jan 23 2019, 6:33 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptJan 23 2019, 6:33 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
arrowd requested review of this revision.Jan 23 2019, 6:33 AM
mwolff requested changes to this revision.Feb 1 2019, 12:32 PM
mwolff added a subscriber: mwolff.

please show the full ASAN report, but the second change looks fine to me - if we get an event during destruction of the tool view action, m_dock may be gone already and thus accessing it may be broken already

kdevplatform/sublime/idealbuttonbarwidget.cpp
63

remove if it doesn't fix anything (this should be done automatically anyways)

84

remove this line then

This revision now requires changes to proceed.Feb 1 2019, 12:32 PM
arrowd updated this revision to Diff 50787.Feb 3 2019, 5:09 PM
arrowd marked 2 inline comments as done.

Remove code used for description.

mwolff accepted this revision.Feb 5 2019, 7:31 AM

please still attach the full ASAN report to your commit - i.e. put it into your commit message

otherwise lgtm

This revision is now accepted and ready to land.Feb 5 2019, 7:31 AM
arrowd added a comment.Feb 5 2019, 8:57 AM

Oh, sorry, I forgot to press "Submit" on Phab and thought I posted ASan log here.

Do you still want me to include it in the commit message, or post it here?

kfunk added a subscriber: kfunk.Feb 5 2019, 9:29 AM

Commit message ideally. Truncate the report where applicable.

This revision was automatically updated to reflect the committed changes.