#647 OWLMaker crashes when creating non-existing file from the MRU list

[Vidar Hasfjord]

This issue occurs when the recreation of the file fails. For example, the parent of the file in question has been moved or removed, or the user has no write permission to the folder.

To reproduce:

1. Create a test file "test.txt" in a new folder "Test".
2. Open the file in OWLMaker.
3. Observe that the MRU list on the File menu now includes "test.txt".
4. Delete the "Test" folder.
5. In OWLMaker, select "test.txt" from the MRU list on the File menu.
6. Message box is shown: "File '...' doesn't exist. Create it?" (Yes/No).
7. Select Yes.
8. Observe the following:
   • Build 8642 (latest release): Crash; no message, just immediate shutdown.
   • Build 7704 and 7498: "Unable to read file '...' from disk" shown twice, followed by empty editor titled "File - Untitled".
   • Build 7349-6049: Same behaviour, but message shown once only.
     
     

Proposed resolution

If the parent folder of the MRU file is not valid, do not ask the user whether to recreate the file. Just show a message informing the user that the file no longer exists, and just remove the MRU entry.

[Ognyan Chernokozhev]

It seems to me part of the problem is that this code is buried too deep in the DocView framework and does not allow user code an easy way to catch and handle the exceptions.

Thus, DocView should handle all these errors gracefully and not throw unhandled exceptions that would bring the whole application down.
Or there should be some easy mechanism that would allow user code to be notifed and for it to decide how to handle the error condition.

[Vidar Hasfjord]

@jogybl wrote:

part of the problem is that this code is buried too deep in the DocView framework and does not allow user code an easy way to catch and handle the exceptions.

While the exception handling within DocView may be poor and need improvement, the faulty logic for this particular problem is implemented in TOWLMakerApp::CmOpenRecentFile.

Draft proposed fix (barely tested):

auto TOWLMakerApp::CmOpenRecentFile(TParam1 p1, TParam2) -> TResult
{
  const auto w = GetMainWindow(); PRECONDITION(w);
  const auto menuItemId = static_cast<int>(p1);
  const auto menuItemText = GetMenuText(menuItemId);
  const auto msgCaption = _T("MRU file open error");
  const auto shouldRecreate = [&]
  {
    const auto m = _T("File \"") + menuItemText + _T("\" doesn't exist. Create it?");
    const auto r = w->MessageBox(m, msgCaption, MB_YESNO | MB_ICONQUESTION);
    return r == IDYES;
  };  
  const auto fail = [&](const char* msg)
  {
    RemoveMenuChoice(menuItemText);
    throw runtime_error{msg};
  };

  try // Don't let exceptions escape handler.
  {
    const auto f = filesystem::path{menuItemText};
    if (filesystem::exists(f))
    {
      OpenFile(menuItemText);
    }
    else if (!filesystem::exists(f.parent_path()))
    {
      fail("File and parent folder no longer exist.");
    }
    else if (!shouldRecreate())
    {
      RemoveMenuChoice(menuItemText);
    }
    else if (ofstream{f}) // Recreate OK?
    {
      OpenFile(menuItemText);
    }
    else // Recreation failed.
    {
      fail("Attempt to recreate MRU file failed.");
    }
  }
  catch (const TDiagException&) { throw; } // Let diagnostics through.
  catch (const exception& x)
  { 
    FlushQueue(); // Remove any pending WM_QUIT message.
    w->EnableWindow(true);
    w->MessageBox(to_tstring(x.what()), msgCaption, MB_OK | MB_ICONERROR); 
  }
  return 0;
}

See "Exceptions and OWLNext | Exceptions in event handlers".

[Ognyan Chernokozhev]

Fix looks good.

[Vidar Hasfjord]

By the way, the reason no message box is shown in the latest release (build 8642) is that the overhauled version of TCoolEdit attempts to save its configuration during stack unwinding after the internal exception caused by the failure to open the MRU file (after the failure to recreate it).

At that point, the editor’s state has already been torn down. In particular, GetApplication returns nullptr inside GetConfigFile_, which leads to a memory access violation and immediate shutdown before any diagnostic dialog can be shown.

This unwinding problem would be indirectly resolved by fixing [bugs:#645], i.e. by not saving the configuration on editor close but instead saving it immediately when configuration changes occur.

[Vidar Hasfjord]

@jogybl wrote:

Fix looks good.

Super! While preparing the patch, I noticed that OWLMaker produces warnings in TSourceCodeDownloadDlg::Find and TClearOutputFoldersDlg::Find ("C4458: declaration of 'TMyClass' hides class member"). The same warning has been circumvented in TOWLMakerApp::Find, so for consistency, you may want to use the same workaround to eliminate the remaining warnings as well.

Note that these ugly workarounds can be eliminated if the OWLNext base classes are rewritten without response table macros as well (a change that has been implemented throughout Owlet).

Index: ClearOutputFoldersDlg.cpp
===================================================================
--- ClearOutputFoldersDlg.cpp   (revision 8827)
+++ ClearOutputFoldersDlg.cpp   (working copy)
@@ -147,7 +147,8 @@
     // Implement and search response table.
     //
     eventInfo.Object = this; // Important for correct dispatch.

-    using TMyClass = TClearOutputFoldersDlg; // Alias used by response table macros.
+#   define TMyClass TClearOutputFoldersDlg // using TMyClass = TClearOutputFoldersDlg; // Alias used by response table macros. NOTE: Macro circumvents warning C4458: declaration of 'TMyClass' hides class member (in base class).
+
     using T = TResponseTableEntry;
     static const auto responseTable =
     {
Index: SourceCodeDownloadDlg.cpp
===================================================================
--- SourceCodeDownloadDlg.cpp   (revision 8827)
+++ SourceCodeDownloadDlg.cpp   (working copy)
@@ -145,7 +145,7 @@
   // Implement and search response table.
   //
   eventInfo.Object = this; // Important for correct dispatch.
-  using TMyClass = TSourceCodeDownloadDlg; // Alias used by response table macros.
+# define TMyClass TSourceCodeDownloadDlg // using TMyClass = TSourceCodeDownloadDlg; // Alias used by response table macros. NOTE: Macro circumvents warning C4458: declaration of 'TMyClass' hides class member (in base class).
   using T = TResponseTableEntry;
   static const auto responseTable = 
   {

PS. There is a new conversion warning on the OWLNext trunk as well that should be eliminated, assuming you still go for clean builds [feature-requests:#248].

Warning C4267: conversion from 'size_t' to 'DWORD', possible loss of data, in "C:\OWLNext\trunk\source\owlcore\modversi.cpp", line 347

Index: source/owlcore/modversi.cpp
===================================================================
--- source/owlcore/modversi.cpp (revision 8827)
+++ source/owlcore/modversi.cpp (working copy)
@@ -344,7 +344,7 @@
 TModuleVersionInfo::GetLanguageName(uint lang)
 {
   tstring langStr(128, _T('\0'));

-  TVersion::VerLanguageName(uint(lang), &langStr[0], langStr.size());
+  TVersion::VerLanguageName(uint(lang), &langStr[0], static_cast<DWORD>(langStr.size()));
   return &langStr[0]; // Don't return trailing null-characters.
 }