Bug 134499 - kst freezes when calling data manager
Summary: kst freezes when calling data manager
Status: RESOLVED FIXED
Alias: None
Product: kst
Classification: Applications
Component: general (show other bugs)
Version: 1.x
Platform: unspecified Solaris
: VHI crash
Target Milestone: ---
Assignee: Adam Treat
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-09-22 17:03 UTC by Nicolas Brisset
Modified: 2007-04-03 20:17 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Files to reprodice the bug (4.66 KB, application/octet-stream)
2006-09-22 17:07 UTC, Nicolas Brisset
Details
Unlock patch (535 bytes, patch)
2006-09-22 18:51 UTC, Adam Treat
Details
Patch to fix deadlock (2.07 KB, patch)
2007-04-03 19:12 UTC, Adam Treat
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Brisset 2006-09-22 17:03:51 UTC
Version:           1.3.0 (using KDE 3.4.0, compiled sources)
Compiler:          gcc version 3.4.3
OS:                SunOS (sun4u) release 5.8

I have downloaded and installed kst 1.3.0 and was about to deploy it. I thought I'd test it with one user before, and installed it alongside svn revision 576925. And guess what happened ? After just a few minutes he came to me and said: there's a bug!

And indeed, we have isolated the soon-to-be attached test case which *freezes* kst (which needs to be killed) when trying to show the datamanager with 1.3.0, whereas it works fine with svn revision 576925. Note that I am not completely sure about the svn revision here, as I may have updated the sources and not recompiled yet (couldn't the "About kst" dialog display the svn revision, I believe with a svn:keywords expansion within a Qstring that should be easy to do... Unless the information is already here and I just missed it ?).

The .kst file was partly script/hand-generated. It looks OK to me, but that may something to keep in mind.
Comment 1 Nicolas Brisset 2006-09-22 17:06:00 UTC
I forgot to mention that it takes *much longer* for the curves to appear than with the recent svn snapshot.
Comment 2 Nicolas Brisset 2006-09-22 17:07:45 UTC
Created attachment 17879 [details]
Files to reprodice the bug
Comment 3 George Staikos 2006-09-22 17:13:29 UTC
There are almost no differences between trunk and 1.3.0 so I suspect a bad build.
Comment 4 Nicolas Brisset 2006-09-22 17:18:29 UTC
I know there aren't many differences, but there were locking issues until very recently and I suspect some cases have been missed. An unclean build is always a possibility, but I built 1.3.0 from a clean tarball, both on Linux and Solaris and in both cases I get the problem. Since it's a standard release build, I don't get much info in the backtrace but here is a hint:

#0  0x006857a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0x00617790 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib/tls/libpthread.so.0
#2  0x004db7dd in KstWaitCondition::wait () from /ldk/0/soft/kde3.5/lib/libkst.so.1
#3  0x004bb023 in KstRWLock::readLock () from /ldk/0/soft/kde3.5/lib/libkst.so.1
#4  0x00427717 in KstDataObject::readLock () from /ldk/0/soft/kde3.5/lib/libkstmath.so.1
#5  0x00282dfd in KstObjectItem::update () from /ldk/0/soft/kde3.5/lib/libkstapp.so.1
#6  0x00281e8b in KstObjectItem::KstObjectItem () from /ldk/0/soft/kde3.5/lib/libkstapp.so.1
#7  0x00287b70 in KstDataManagerI::update () from /ldk/0/soft/kde3.5/lib/libkstapp.so.1
#8  0x00287890 in KstDataManagerI::show_I () from /ldk/0/soft/kde3.5/lib/libkstapp.so.1
#9  0x0028b5ec in KstDataManagerI::qt_invoke () from /ldk/0/soft/kde3.5/lib/libkstapp.so.1
#10 0x02d15a7c in QObject::activate_signal () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#11 0x02d158a4 in QObject::activate_signal () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#12 0x08f14880 in KAction::activated () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#13 0x08f13af1 in KAction::slotActivated () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#14 0x08f13ed2 in KAction::slotButtonClicked () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#15 0x08f14b1c in KAction::qt_invoke () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#16 0x02d15a7c in QObject::activate_signal () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#17 0x08fad0a6 in KToolBarButton::buttonClicked () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#18 0x08fabbe3 in KToolBarButton::mouseReleaseEvent () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#19 0x02d4ba17 in QWidget::event () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#20 0x08faca8a in KToolBarButton::event () from /ldk/0/soft/kde3.5/lib/libkdeui.so.4
#21 0x02cb99ff in QApplication::internalNotify () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#22 0x02cb90f4 in QApplication::notify () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#23 0x0074f2bc in KApplication::notify () from /ldk/0/soft/kde3.5/lib/libkdecore.so.4
#24 0x02c50220 in QETWidget::translateMouseEvent () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#25 0x02c4deac in QApplication::x11ProcessEvent () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#26 0x02c64c34 in QEventLoop::processEvents () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#27 0x02ccbbf8 in QEventLoop::enterLoop () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#28 0x02ccbaa8 in QEventLoop::exec () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#29 0x02cb9c51 in QApplication::exec () from /usr/lib/qt-3.3/lib/libqt-mt.so.3
#30 0x08053a11 in main ()
Comment 5 Adam Treat 2006-09-22 17:22:27 UTC
I'm getting the same hang with Kst from trunk...
Comment 6 Adam Treat 2006-09-22 18:51:09 UTC
Created attachment 17881 [details]
Unlock patch

This bug seems to be related to #133311 and commit #581128.

It seems that the scalars are locked even after the call to equation update. 
So, the patch just unlocks them after the update since there doesn't seem to be
an unlock anywhere in enodes.cpp.  I'm not sure if this is correct as perhaps
they are supposed to be unlocked somewhere else.
Comment 7 Nicolas Brisset 2006-09-25 11:05:25 UTC
The patch seems to work here :-) Thanks, Adam ! 
Comment 8 Nicolas Brisset 2006-09-29 11:53:58 UTC
Has this patch been included ? It seems to work fine, so if it is conceptually and syntactically correct, we should apply it and close this bug...
Comment 9 Adam Treat 2006-10-02 17:32:00 UTC
Right now this patch reverts the fix for bug #133311

Investigating...
Comment 10 Adam Treat 2006-10-07 00:13:25 UTC
SVN commit 593145 by treat:

* Do not allow equation parsing to proceed when
the equation references non-existent objects, whether
they be scalars, vectors or else.

BUG: 133311
BUG: 134499
BUG: 134996



 M  +18 -9     enodes.cpp  
 M  +5 -5      enodes.h  
 M  +10 -7     kstequation.cpp  


--- trunk/extragear/graphics/kst/src/libkstmath/enodes.cpp #593144:593145
@@ -93,7 +93,8 @@
 }
 
 
-void Node::collectObjects(KstVectorMap&, KstScalarMap&, KstStringMap&) {
+bool Node::collectObjects(KstVectorMap&, KstScalarMap&, KstStringMap&) {
+  return true;
 }
 
 
@@ -128,9 +129,11 @@
 }
 
 
-void BinaryNode::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
-  _left->collectObjects(v, s, t);
-  _right->collectObjects(v, s, t);
+bool BinaryNode::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
+  bool ok = true;
+  ok = _left->collectObjects(v, s, t) ? ok : false;
+  ok = _right->collectObjects(v, s, t) ? ok : false;
+  return ok;
 }
 
 
@@ -600,8 +603,8 @@
 }
 
 
-void Function::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
-  _args->collectObjects(v, s, t);
+bool Function::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
+  return _args->collectObjects(v, s, t);
 }
 
 
@@ -651,10 +654,12 @@
 }
 
 
-void ArgumentList::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
+bool ArgumentList::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
+  bool ok = true;
   for (Node *i = _args.first(); i; i = _args.next()) {
-    i->collectObjects(v, s, t);
+    ok = i->collectObjects(v, s, t) ? ok : false;
   }
+  return ok;
 }
 
 
@@ -853,7 +858,7 @@
 }
 
 
-void Data::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
+bool Data::collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t) {
   if (_isEquation) {
     if (_equation) {
       _equation->collectObjects(v, s, t);
@@ -862,7 +867,11 @@
     v.insert(_tagName, _vector);
   } else if (_scalar && !s.contains(_tagName)) {
     s.insert(_tagName, _scalar);
+  } else {
+    KstDebug::self()->log(i18n("Equation has unknown object [%1].").arg(_tagName), KstDebug::Error);
+    return false;
   }
+  return true;
 }
 
 
--- trunk/extragear/graphics/kst/src/libkstmath/enodes.h #593144:593145
@@ -57,7 +57,7 @@
       virtual ~Node();
 
       virtual bool isConst() = 0; // can't be const
-      virtual void collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
+      virtual bool collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
       virtual bool takeVectors(const KstVectorMap& c);
       virtual double value(Context*) = 0;
       virtual void visit(NodeVisitor*);
@@ -75,7 +75,7 @@
       BinaryNode(Node *left, Node *right);
       virtual ~BinaryNode();
 
-      virtual void collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
+      virtual bool collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
       virtual bool takeVectors(const KstVectorMap& c);
       virtual void visit(NodeVisitor*);
       virtual KstObject::UpdateType update(int counter, Context *ctx);
@@ -102,7 +102,7 @@
       double value(Context*) { return 0.0; }
 
       bool isConst();
-      void collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
+      bool collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
       bool takeVectors(const KstVectorMap& c);
       double at(int, Context*);
       Node *node(int idx);
@@ -124,7 +124,7 @@
       bool isPlugin() const;
       bool isConst();
       double value(Context*);
-      void collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
+      bool collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
       bool takeVectors(const KstVectorMap& c);
       KstObject::UpdateType update(int counter, Context *ctx);
       QString text() const;
@@ -186,7 +186,7 @@
 
       bool isConst();
       double value(Context*);
-      void collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
+      bool collectObjects(KstVectorMap& v, KstScalarMap& s, KstStringMap& t);
       bool takeVectors(const KstVectorMap& c);
       KstObject::UpdateType update(int counter, Context *ctx);
       QString text() const;
--- trunk/extragear/graphics/kst/src/libkstmath/kstequation.cpp #593144:593145
@@ -268,9 +268,6 @@
   setDirty();
   _equation = in_fn;
   VectorsUsed.clear();
-  for (KstScalarMap::Iterator i = _inputScalars.begin(); i != _inputScalars.end(); ++i) {
-    (*i)->unlock();
-  }
   _inputScalars.clear();
   _ns = 2; // reset the updating
   delete _pe;
@@ -288,17 +285,23 @@
       ctx.xVector = *_xInVector;
       Equation::FoldVisitor vis(&ctx, &_pe);
       KstStringMap sm;
-      _pe->collectObjects(VectorsUsed, _inputScalars, sm);
-      for (KstScalarMap::Iterator i = _inputScalars.begin(); i != _inputScalars.end(); ++i) {
-        (*i)->readLock();
+      if (_pe->collectObjects(VectorsUsed, _inputScalars, sm)) {
+        _pe->update(-1, &ctx);
+      } else {
+        //we have bad objects...
+        KstDebug::self()->log(i18n("Equation [%1] references non-existent objects.").arg(_equation), KstDebug::Error);
+        _inputScalars.clear();
+        delete (Equation::Node*)ParsedEquation;
+        ParsedEquation = 0L;
+        Equation::mutex().unlock();
       }
-      _pe->update(-1, &ctx);
     } else {
       // Parse error
       KstDebug::self()->log(i18n("Equation [%1] failed to parse.  Errors follow.").arg(_equation), KstDebug::Warning);
       for (QStringList::ConstIterator i = Equation::errorStack.begin(); i != Equation::errorStack.end(); ++i) {
         KstDebug::self()->log(i18n("Parse Error: %1").arg(*i), KstDebug::Warning);
       }
+      _inputScalars.clear();
       delete (Equation::Node*)ParsedEquation;
       ParsedEquation = 0L;
       Equation::mutex().unlock();
Comment 11 Nicolas Brisset 2006-10-11 14:33:01 UTC
> On Monday 09 October 2006 8:07 am, Brisset, Nicolas wrote:
> > Thanks for your work on this and the bugfixes, this problem 
> resulted 
> > in a lot of crashes recently and was becoming quite painful 
> :-) There 
> > is just one thing I'm wondering about: when I load existing .kst 
> > files, I get errors for scalars and vectors used in equations, even 
> > though they are in the file and eventually loaded correctly. I have 
> > the feeling that the loader delays some instantiations, which now 
> > generates a lot of meaningless (since in the end everything 
> is there) 
> > error messages. I don't know how the chain of events works, 
> so maybe 
> > I'm just wrong here, but the fact is I get lots of errors 
> (not warnings, errors) which I think I shouldn't be getting.
> 
> Hi Nicolas,
> 
> I'm not sure if this is the .kst file you are referring to, 
> but the one you included in 
> http://bugs.kde.org/show_bug.cgi?id=134499 referenced two 
> scalars which do not exist anywhere in the .kst file.  Can 
> you please make sure that the .kst files do in fact contain 
> the scalars and vectors?  Otherwise, feel free to send along 
> the .kst files or reproducable bugs and I'll have a look.

In the testcase_134499.kst file from the attachment, there are 2
equations referencing only available objects: all scalars are defined
above in the same file, and V-POSE and V-POSN are vectors produced by
plugins, also defined in the same file. If you look at the objects in
the datamanager you will see that they are all there. What's more, the
log output says:
"11/10/2006 14:21 Equation has unknown object [CST-PSI0_dg].
11/10/2006 14:21 Equation has unknown object [CST-dg2rd].
11/10/2006 14:21 Equation
[[CST-kt2ms]*(COS([CST-PSI0_dg]*[CST-dg2rd])*[V-POSE]+SIN([CST-PSI0_dg]*
[CST-dg2rd])*[V-POSN]+[CST-X0])] references non-existent objects."
and I have just checked, these scalars are defined.

So there is clearly something wrong here, like objects being reported as
unknown whereas they are defined. This is why I suspect delayed
instantiations to cause this...
Comment 12 Duncan Hanson 2007-03-29 23:01:49 UTC
this still happens for me when using scalars in an event monitor. should changes similar to those made for kstequation be made for events also?

duncan.
Comment 13 Adam Treat 2007-03-30 20:50:35 UTC
Duncan, could you provide exact steps to reproduce?  Thanks.
Comment 14 Duncan Hanson 2007-03-30 21:43:52 UTC
Sure,

1. Load some data... "kst -y 1 gyrodata.dat" works for me.
2. Then Data->New Event Monitor. Set expression to [1/First] or something.
3. Now click Ok. Everything will appear to be fine, until you try to enter
the Data Wizard or the Data Manager (possibly other things). This part is
probably why you haven't been able to reproduce yet- sorry, I didn't realize
that there was this requirement when I put in the original report!

Duncan.

On 30 Mar 2007 18:50:36 -0000, Adam Treat <treat@kde.org> wrote:
[bugs.kde.org quoted mail]
Sure,<br><br>1. Load some data... &quot;kst -y 1 gyrodata.dat&quot; works for me.<br>2. Then Data-&gt;New Event Monitor. Set expression to [1/First] or something.<br>3.
Now click Ok. Everything will appear to be fine, until you try to enter
the Data Wizard or the Data Manager (possibly other things). This part
is probably why you haven&#39;t been able to reproduce yet- sorry, I didn&#39;t
realize that there was this requirement when I put in the original
report!
<br><br>Duncan.<br><br><div><span class="gmail_quote">On 30 Mar 2007 18:50:36 -0000, <b class="gmail_sendername">Adam Treat</b> &lt;<a href="mailto:treat@kde.org">treat@kde.org</a>&gt; wrote:</span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
------- You are receiving this mail because: -------<br>You are on the CC list for the bug, or are watching someone who is.<br><br><a href="http://bugs.kde.org/show_bug.cgi?id=134499">http://bugs.kde.org/show_bug.cgi?id=134499
</a><br><br><br><br><br>------- Additional Comments From treat kde org&nbsp;&nbsp;2007-03-30 20:50 -------<br>Duncan, could you provide exact steps to reproduce?&nbsp;&nbsp;Thanks.<br>_______________________________________________<br>Kst mailing list
<br><a href="mailto:Kst@kde.org">Kst@kde.org</a><br><a href="https://mail.kde.org/mailman/listinfo/kst">https://mail.kde.org/mailman/listinfo/kst</a><br></blockquote></div><br>
Comment 15 Adam Treat 2007-03-30 22:57:36 UTC
Thanks, I can confirm.
Comment 16 Adam Treat 2007-04-03 19:12:59 UTC
Created attachment 20162 [details]
Patch to fix deadlock

This fixes the deadlock, but updates are called continuously from
updatethread.cpp
Comment 17 George Staikos 2007-04-03 19:27:52 UTC
Looks okay after a couple fixes:
1) Must remove the i18n() for now.  Just leave out that kstdebug line  
altogether.
2) I think there are some tabs or weird indenting in this patch?

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/
Comment 18 Adam Treat 2007-04-03 20:17:00 UTC
SVN commit 650056 by treat:

* Fix for deadlock

BUG:134499


 M  +6 -2      ksteventmonitor_i.cpp  
 M  +6 -8      ksteventmonitorentry.cpp  


--- trunk/extragear/graphics/kst/src/libkstapp/ksteventmonitor_i.cpp #650055:650056
@@ -187,15 +187,19 @@
   }
 
   EventMonitorEntryPtr event = new EventMonitorEntry(tag_name);
+  event->writeLock();
   fillEvent(event);
 
   if (!event->isValid()) {
+    event->unlock();
     event = 0L;
 
     KMessageBox::sorry(this, i18n("There is a syntax error in the equation you entered."));
     return false;
   }
 
+  event->unlock();
+
   KST::dataObjectList.lock().writeLock();
   KST::dataObjectList.append(event.data());
   KST::dataObjectList.lock().unlock();
@@ -255,10 +259,10 @@
   } else if (_w->radioButtonLogError->isChecked()) {
     emPtr->setLevel(KstDebug::Error);
   }
-  
+
   emPtr->reparse();
   emPtr->unlock();
-  
+
   return true;
 }
 
--- trunk/extragear/graphics/kst/src/libkstapp/ksteventmonitorentry.cpp #650055:650056
@@ -118,6 +118,7 @@
 
 
 bool EventMonitorEntry::reparse() {
+  Q_ASSERT(myLockStatus() == KstRWLock::WRITELOCKED);
   _isValid = false;
   if (!_event.isEmpty()) {
     QMutexLocker ml(&Equation::mutex());
@@ -128,15 +129,12 @@
       Equation::Context ctx;
       Equation::FoldVisitor vis(&ctx, &_pExpression);
       KstStringMap stm;
-      _pExpression->collectObjects(_vectorsUsed, _inputScalars, stm);
-
-      for (KstScalarMap::ConstIterator i = _inputScalars.begin(); i != _inputScalars.end(); ++i) {
-        if ((*i)->myLockStatus() == KstRWLock::UNLOCKED) {
-          (*i)->readLock();
-        }
-      }
-
+      if (_pExpression->collectObjects(_vectorsUsed, _inputScalars, stm)) {
       _isValid = true;
+      } else {
+        //we have bad objects...
+        delete (Equation::Node*)ParsedEquation;
+      }
     } else {
       delete (Equation::Node*)ParsedEquation;
     }