Viewer: a failed live property update no longer frees the cached property (#217)

PropertyTask::updateProperty() applies a running job's message to the
object getProperty() has cached, and on any failure (a GEOMTRACE step whose
size disagrees with its header, a step the data server cannot deliver, an
empty value) it deleted that object while p_properties kept pointing at it.
The next getProperty("GEOMTRACE") returned freed memory: loadStep() read a
garbage step size ("Bad Geomtrace") and then SIGSEGV, the signature of
#217; the trace panel crashed in dynamic_cast. Reproduced headless with
tests/apps/geomtrace_stress.py (gdb backtrace in __dynamic_cast on the
freed property). A failed update now leaves the cached property unchanged,
and a parse exception there is caught as getProperty() already does.

A second crash found by the same driver: PlaybackControl re-arms a one-shot
wxTimer every step and never deleted it, so removing the property panels
while a trace animates (a calculation reset, urlStateMCB) let the pending
tick reach the destroyed control (SIGSEGV in wxTimerImpl::SendEvent).
The destructor now deletes the timer; NModePanel's animation timer the same.

GeomTracePropertyPanel: a playback position past the end of a trace that
has since changed is clamped instead of collapsing the atoms to the
origin, the plot cursor is only set where the plotted series has a point
(TEVEC may be shorter than GEOMTRACE), refresh() before initialization
and a property without a registry entry no longer dereference null.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFPK14wBcLD1s4dYUowpP4

Author: andy

docs/claude/wx-viewer/index.md

@@ -39,3 +39,4 @@ Read this before changing any dialog, panel, sizer, grid, ewx control or the Ope
 - [The Vibrational Frequencies graph is `SpectrumCanvas` over the wx-free `VibSpectrum` model (#214)](vibrational-spectrum-canvas-214.md)
 - [Builder panel layouts (View > Panel layout): every pane stays an AUI pane; the one-column modes hide the inactive tab's panes](builder-panel-layouts-one-column.md)
 - [A structure opened from a file is not a calculation: `AbstractPropCalculation::getProperty()` throws](structure-file-calculations-throw-on-getproperty.md)
+- [A failed live property update must not free the cached property; a panel's timer must die with the panel (#217)](property-cache-and-panel-timers-outlive-their-objects.md)

docs/claude/wx-viewer/property-cache-and-panel-timers-outlive-their-objects.md

@@ -0,0 +1,27 @@
+---
+type: pitfall
+title: "A failed live property update must not free the cached property; a panel's timer must die with the panel (#217)"
+area: wx-viewer
+paths: ["src/dsm/edsiimpl/PropertyTask.C", "src/wxgui/wxtools/PlaybackControl.C", "src/apps/builder/NModePanel.C", "src/apps/builder/GeomTracePropertyPanel.C", "tests/apps/geomtrace_stress.py"]
+issues: [217]
+---
+**`PropertyTask::updateProperty()` works on the cached object.**
+`getProperty()` puts everything it returns into `p_properties`, so the
+object a running job's message is applied to is the one every panel and
+`loadStep()` will read next. When an update failed (a step whose size
+disagrees with its header, a step the data server could not deliver, an
+empty value), it used to `delete` that object and leave the cache entry
+in place. The next `getProperty("GEOMTRACE")` then returned freed memory:
+"Bad Geomtrace" (garbage step size) followed by SIGSEGV, or a crash in
+`dynamic_cast`. A failed update now returns 0 and leaves the cached
+property as it was.
+
+**Timers owned by a panel are not stopped by wx when the panel goes.**
+`PlaybackControl` re-arms a one-shot `wxTimer` for every playback step and
+never deleted it; `removePropertyPanels()` (a calculation reset while its
+trace or vibration is animating) destroyed the panel and the pending
+timer then delivered its tick to freed memory. Delete the timer in the
+destructor (`~wxTimer` stops it).
+
+`tests/apps/geomtrace_stress.py` reproduces both (with `--gdb` for a
+backtrace); the Builder's `gt...` scene commands are its driver.

src/apps/builder/GeomTracePropertyPanel.C

@@ -233,6 +233,7 @@ wxWindow * GeomTracePropertyPanel::GetTearableContent()
 
 void GeomTracePropertyPanel::refresh()
 {
+  if (!p_playback) return;   // not initialized yet
   p_playback->SetStopIndex(numSteps()-1);
   fillPlot();
 }
@@ -369,6 +370,13 @@ void GeomTracePropertyPanel::OnValidation(wxCommandEvent& event)
 // we don't want/need undo
 void GeomTracePropertyPanel::processStep(int step)
 {
+  // The playback position can outlive the trace it was set for (a reload
+  // after a message from a running job), and a trace can be empty.
+  int steps = numSteps();
+  if (steps == 0) return;
+  if (step < 0) step = 0;
+  if (step >= steps) step = steps - 1;
+
   WxVizToolFW& fw = getFW();
   IPropCalculation *expt = getCalculation();
   SGContainer& sg = fw.getSceneGraph();
@@ -435,7 +443,12 @@ void GeomTracePropertyPanel::processStep(int step)
      }
   }
 
-  p_plotCtrl->SetCursorDataIndex(0, step);
+  // The plotted series (TEVEC, ...) may have fewer points than the trace.
+  wxPlotData *curve = p_plotCtrl->CurveIndexOk(0) ?
+          p_plotCtrl->GetDataCurve(0) : NULL;
+  if (curve && step < curve->GetCount()) {
+    p_plotCtrl->SetCursorDataIndex(0, step);
+  }
 
   // Issue #99: drive the repaint directly instead of relying on Open
   // Inventor's change notification, which stops firing after the first
@@ -493,7 +506,7 @@ string GeomTracePropertyPanel::getTracePropKey() const
   for (name = names.begin(); name != names.end(); ++name) {
     string key = *name;
     const Property_Ref *propref = PropFactory::getPropRef(key);
-    if (propref->classType == TProperty::PROPTSVECTABLE) {
+    if (propref && propref->classType == TProperty::PROPTSVECTABLE) {
       return key;
     }
   }

src/apps/builder/NModePanel.C

@@ -128,6 +128,8 @@ NModePanel::NModePanel(IPropCalculation *calculation,
 
 NModePanel::~NModePanel()
 {
+   //  The animation timer must not fire into a destroyed panel.
+   delete p_timer;
    //  The window's canvas calls back into this panel.
    if (p_spectrumPop) p_spectrumPop->setClickHandler(0);
    if (p_spectrumFrame) p_spectrumFrame->Destroy();

src/dsm/edsiimpl/PropertyTask.C

@@ -268,10 +268,12 @@ TProperty* PropertyTask::updateProperty(const string& name, const string& value)
   //If property is not on DAV server then return null because this property is 
   //probably a time - series property that is being cached.
   // uhhhh what???
+  //  From here on `property` is the cached object (getProperty() caches
+  //  whatever it returns), so a failed update must leave it alone: deleting
+  //  it left p_properties pointing at freed memory, and the next reader of
+  //  the property -- a geometry step, the trace panel -- crashed (#217).
   if (localValue.empty()) {
-    delete property;
-    property = NULL;
-    return property;
+    return 0;
   }
 
 
@@ -336,11 +338,15 @@ TProperty* PropertyTask::updateProperty(const string& name, const string& value)
   // This may change, but for now, PROPERTY_OUTPUT files are not parsed
   PropertyDoc doc;
   if (!isOutputFile) {
-    doc.parse(localValue.c_str(), false);
+    try {
+      doc.parse(localValue.c_str(), false);
+    } catch (...) {
+      // A step the data server could not deliver leaves the URL here.
+      return 0;
+    }
   }
 
-  if ( (doc.empty() && !isOutputFile) || !property ) {
-    delete property;
+  if (doc.empty() && !isOutputFile) {
     property = NULL;
   } 
   else {
@@ -373,7 +379,6 @@ TProperty* PropertyTask::updateProperty(const string& name, const string& value)
       default: ok = false; break;
     };
     if (!ok) {
-      delete property;
       property = NULL;
     } 
     else {

src/wxgui/wxtools/PlaybackControl.C

@@ -106,6 +106,14 @@ PlaybackControl::PlaybackControl(wxWindow* parent, wxWindowID id,
 
 PlaybackControl::~PlaybackControl()
 {
+  // A one-shot is pending for as long as playback runs; one that fired
+  // after the panel was destroyed would deliver its tick to freed memory.
+  delete p_timer;
+  if (p_delayInternal) delete p_delay;
+  if (p_startIndexInternal) delete p_startIndex;
+  if (p_stopIndexInternal) delete p_stopIndex;
+  if (p_stepSizeInternal) delete p_stepSize;
+  if (p_currentStepInternal) delete p_currentStep;
 }