#634 Double Condemn causes hang

[Jörg Prenzing]

Found in OWLNext 7.0.19.

TApplication::Condemn: The last condemned window is not checked for double condemnation.

Example

TWindow* toCondemn = 0xdeadbeef;
TApplication::Condemn(toCondemn);
TApplication::Condemn(toCondemn);
TApplication::DeleteCondemned(); // gets stuck in infinite loop

First call to Condemn just sets TApplication::CondemnedWindows. Second call loops through all siblings to CondemnedWindows, but since there aren't any, it never enters the loop body and never checks for the already condemned window. So it sets toCondemn as sibling to itself, and any further iterations through this list will loop infinitely.

Proposal for fix: Check eol != win before calling eol->SetNext(win). This won't trigger the CHECK macro though, but it will keep the CondemnedWindows list intact.

--- a/owlnext/7.0.19/source/owlcore/applicat.cpp
+++ b/owlnext/7.0.19/source/owlcore/applicat.cpp
@@ -1271,7 +1271,8 @@ TApplication::Condemn(TWindow* win)
         CHECK(!"Double condemn is not nice!");
         return;  //already condemned
       }
-    eol->SetNext(win);
+    if(eol!=win)
+      eol->SetNext(win);
   }
   else{
     CondemnedWindows = win;

[Vidar Hasfjord]

This ticket is a duplicate of ticket [bugs:#368] "The same TWindow object can be condemned more than once". Unfortunately, the fix [r3773] was incomplete, in that the last entry in the CondemnedWindows list was not checked, as pointed out by this ticket.

On further code review, there is another issue: If a double condemnation is detected, the next-pointer of the window has already been cleared, thereby truncating the list.

I propose the following fix for both issues:

void TApplication::Condemn(TWindow* win)
{
  PRECONDITION(win);
  TRACEX(OwlApp, 1, _T("Condemning window @") << (void*)win << *win);

  // Insert the new window to be deleted at the end of the list.
  //
  if (!CondemnedWindows) 
  {
    CondemnedWindows = win; // Simply set head.
  }
  else
  {
    // Find end of list (while also checking for double condemnation).
    //
    TWindow* eol = nullptr;
    for (TWindow* w = CondemnedWindows; w; eol = w, w = w->Next())
      if (w == win)
      {
        WARNX(OwlApp, true, 0, _T("TApplication::Condemn: Double condemnation, win: ") << *win);
        return; // Ignore double condemnation.
      }
    CHECK(eol);
    eol->SetNext(win); // Update end of list.
  }
  win->SetNext(nullptr); // Terminate list.
  win->SetParent(nullptr);
}

Note that the code now WARNX rather than CHECK for double condemnation, since the issue is safely handled.

Please review.

[Vidar Hasfjord]

My proposed fix has now been committed on the trunk [r8588] and merged into branches 6.36, 6.44, 7 and Owlet [r8589].

(Edit: A regression has been fixed on the trunk [r8590] and branches [r8591].)

Please review.

[Jörg Prenzing]

I did some testdriving with r8588/r8859 and experienced intermitting crashes in TWindow::RemoveChild() while condemning TWindow-Instances.

r8590/r8591 seem to fix this, Test cases now run smoothly and without incident.

[Vidar Hasfjord]

@jprenz wrote:

Test cases now run smoothly and without incident.

Super! Thanks for testing and feedback, and thanks for reporting this issue.

Upcoming updates will include the fix.