#543 Exception in constructor of TView subclass causes crash

[Hideaki KODATSU]

In OWLNext 7, TDocManager::CreateDoc deletes its document pointer by using std::unique_ptr when an exception is thrown indirectly by InitDoc. InitDoc indirectly creates an instance of a class derived from TView. When an exception is thrown by a constructor of the TView subclass, because the TView instance part has been fully created, TView::~TView is called and its document instance is deleted (provided dtAutoDelete is set in the document template and TView::IsOK returns true). In this case, TDocManager::CreateDoc deletes the document pointer again.

Here is to recreate this situation:

1. Use "examples\richeditor",
2. apply the following patch,
3. build, run,
4. and create a new text file in the application.

--- App.cpp.org 2021-05-19 02:43:56.000000000 +0900
+++ App.cpp 2022-12-16 20:13:58.501763600 +0900
@@ -484,6 +484,12 @@

 int OwlMain(int, _TCHAR*[])
 {

-  TRichEditorApp app;
-  return app.Run();
+  try {
+    TRichEditorApp app;
+    return app.Run();
+  }
+  catch (const std::exception& x) {
+    ::MessageBox(0, x.what(), "Exception", MB_ICONSTOP | MB_OK);
+    return 1;
+  }
 }
--- Editor.cpp.org  2020-05-19 00:21:14.000000000 +0900
+++ Editor.cpp  2022-12-16 18:13:28.820063000 +0900
@@ -11,7 +11,9 @@

 TPlainEditor::TPlainEditor(TDocument& doc, TWindow* parent)
   : TEditView(doc, parent)
-{}
+{

+    throw std::runtime_error("an error.");
+}

 //
 // Paint routine for Window, Printer, and PrintPreview for a TEditView client.

Workaround

Don't throw an exception from a constructor of a class derived from OWLNext, unless you are confident that OWLNext handles it well. Generally, the OWL code is not exception safe. See article Exceptions and OWLNext in our Knowledge Base.

In particular, TView is not designed to handle exceptions in constructors, so you should use another mechanism to signal that the object is in an invalid state. As it happens, TView provides member function NotOK to "flag errors in creation" and thereby set the object to an invalid state. Function IsOK is used to test valid state, and should only return "true if successfully created".

In this case, calling NotOK before throwing the exception will circumvent the double-delete issue (see the implementation of TDocument::DetachView, which returns false if the view is invalid, which in turn prevents ~TView from deleting the document). However, the exception may cause other issues, so if you use this workaround, study the OWLNext source code and check that all the implicated code works well with the exception.

TPlainEditor::TPlainEditor(TDocument& doc, TWindow* parent)
  : TEditView(doc, parent)
{
  NotOK();
  throw std::runtime_error("an error.");
}

[Vidar Hasfjord]

Hi Hideaki,

Thanks for reporting this issue. This seems to be a regression related to my fixes for [bugs:#344] and [bugs:#346]. I will look into it.

That said, the OWL code is generally not exception safe, so throwing an exception in the TView constructor is probably not a good idea. See my article Exceptions and OWLNext.

[Vidar Hasfjord]

I've now analysed the issue. I have added a Workaround section in the ticket, which explains the underlying library design. In particular, a TView subclass is supposed to call TView::NotOK on construction failure.

Since this is not strictly a bug in OWLNext, but a consequence of its design, should we leave it as is or attempt to improve the implementation, i.e. make the code more robust in face of exceptions?

I don't see an obvious simple fix, since an exception in a constructor of a TView subclass will call ~TView (as pointed out in the ticket), and the destructor deletes the document unless NotOK was called (and provided dtAutoDelete is set). So there is no place to intervene and call NotOK automatically, as to prevent the document deletion.

An (ugly?) hack would be to temporarily turn off dtAutoDelete wherever construction occurs. For example, in TDocManager::CreateView:

TView* TDocManager::CreateView(TDocument& doc, TDocTemplate* tpl)
{
  //...
  const auto constructView = [&]
  {
    // Temporarily unset the dtAutoDelete flag. This prevents the document from being deleted in
    // TView::~TView, in case the constructor of the TView subclass throws an exception without
    // first calling TView::NotOK to signal that construction failed. See [bugs:#543].
    //
    struct TTemporaryAutoDeleteFlagDisabler
    {
      TDocTemplate& Template;
      bool HasAutoDeleteFlag;

      TTemporaryAutoDeleteFlagDisabler(TDocTemplate& tpl)
        : Template{tpl}, HasAutoDeleteFlag{(tpl.GetFlags() & dtAutoDelete) != 0}
      {
        if (HasAutoDeleteFlag)
          Template.ClearFlag(dtAutoDelete);
      }

      ~TTemporaryAutoDeleteFlagDisabler()
      {
        if (HasAutoDeleteFlag)
          Template.SetFlag(dtAutoDelete);
      }
    }
    disabler{*tpl};
    return tpl->ConstructView(doc);
  };
  auto view = constructView();
  WARNX(OwlDocView, !view, 0, _T("CreateView(): ConstructView call failed"));
  //...
}

However, a better long-term solution is perhaps to redesign the code, replacing the messy ownership model based on flags and object state (dtAutoDelete and IsOK) by shared ownership using std::shared_ptr. This would simplify the code greatly, eliminating the tricky clean-up issue in TView::~TView. This solution is something I might experiment with in Owlet.

A less radical refinement of the current design is to change the TView constructor signature to not have the document as a parameter, and instead require that the document is assigned using TView::SetDocument after successful construction. This would allow us to rewrite TView::~TView to distinguish calls before and after successful construction. However, this design is not very robust, since there is the danger that derived classes may just call SetDocument in their constructor, thereby thwarting this refinement.

The attached patch implements the latter solution, should you want to test it with your code. The patch is applicable to the trunk, revision 6232. Note that the patch does not update all the examples — just RichEditor.

[Hideaki KODATSU]

The workaround works well. Thank you.