MantisBT - ParaView
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0013120ParaView(No Category)public2012-04-24 02:572012-10-29 17:04
ReporterZhan 
Assigned ToSebastien Jourdain 
PriorityhighSeverityminorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version3.14 
Target VersionFixed in Version3.98.0 
ProjectRelease
Topic Name13120-fix-delete-macro
Typecrash
Summary0013120: Paraview crashed when deleting an added python macro from Macro Menu
DescriptionI have compiled ParaView 3.10.1 and 3.14.0 as 32bit on Windows 7 with MS VS 2008, and I found it would crash when I tried to delete a macro I added.

After debugging for some time, I found there's bug in the following codes:

for ( ; itr != this ->Internal->DeleteActionMap.constEnd(); ++itr )
    {
    if (itr.value() == action)
      {
          QString filename = itr.key();
          pqPythonMacroSupervisor::removeStoredMacro(filename);
          pqPythonMacroSupervisor::removeMacro(filename);
       }
    }
QMap's removing actions has different behavior on VS Compiler, and removing one element in this loop would cause dangling pointer problem.

I have changed the codes to the following(do not increment in for clauses) to fix this bug:
for ( ; itr != this->Internal->DeleteActionMap.constEnd();)
    {
    if (itr.value() == action)
      {
      QString filename = (itr++).key();
      pqPythonMacroSupervisor::removeStoredMacro(filename);
      pqPythonMacroSupervisor::removeMacro(filename);
      }
    else
    {
        ++itr;
    }
    }
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2012-04-24 02:57ZhanNew Issue
2012-07-05 14:38LawrenceNote Added: 0028744
2012-07-06 11:59LawrenceNote Added: 0028754
2012-07-19 09:10Sebastien JourdainAssigned To => Sebastiennn Jourdain
2012-07-19 09:10Sebastien JourdainTopic Name => 13120-fix-delete-macro
2012-07-19 09:10Sebastien JourdainStatusbacklog => gatekeeper review
2012-07-19 09:10Sebastien JourdainResolutionopen => fixed
2012-07-24 10:47Utkarsh AyachitFixed in Version => git-master
2012-07-24 10:48Utkarsh AyachitStatusgatekeeper review => customer review
2012-07-24 10:48Utkarsh AyachitNote Added: 0028898
2012-07-25 14:55Alan ScottNote Added: 0028935
2012-07-25 14:55Alan ScottStatuscustomer review => closed
2012-10-29 17:04Utkarsh AyachitFixed in Versiongit-master => 3.98.0

Notes
(0028744)
Lawrence   
2012-07-05 14:38   
I noticed pqPythonMacroSupervisor::removeMacro calls
this->Internal->DeleteActionMap.remove(fileName)
so is the iterator even still guaranteed to be valid?
(0028754)
Lawrence   
2012-07-06 11:59   
The following is a more robust fix.
(Started with 3.14.1 code, Fixes Dangling Iterator problem. Tested on Win7 64bit build of 3.14.1 - deleting macros no longer crashes Paraview)

void pqPythonMacroSupervisor::onDeleteMacroTriggered()
{
  QObject* action = this->sender();
  QMap<QString, QPointer<QAction> >::const_iterator itr = this->Internal->DeleteActionMap.constBegin();
  // (A more paranoid version would restart the iterator using DeleteActionMap.constBegin()).
  while( itr != this->Internal->DeleteActionMap.constEnd())
    {
    if (itr.value() == action)
      {
      QString filename = itr.key();
      ++itr; // Avoid dangling iterator; increment iterator before removeMacro modifies DeleteActionMap
      // "If you remove items from the map, iterators that point to the removed items will become dangling iterators."
      // - http://qt-project.org/doc/qt-5.0/qmap-iterator.html [^] , potentially causing a crash
      pqPythonMacroSupervisor::removeStoredMacro(filename);
      pqPythonMacroSupervisor::removeMacro(filename);
      }
    else
      {
      ++itr;
      }
    }
}
(0028898)
Utkarsh Ayachit   
2012-07-24 10:48   
merged into master, if applicable.
(0028935)
Alan Scott   
2012-07-25 14:55   
Internal code fix. Closing. (Note that there really isn't enough information here to test.)