fix: degrade malformed legacy XML Session Files instead of throwing

src/LogExpert.Core/Classes/Persister/PersisterXML.cs

@@ -235,7 +235,13 @@ private static PersistenceData LoadInternal (string fileName)
     private static PersistenceData ReadPersistenceDataFromNode (XmlNode node)
     {
         PersistenceData persistenceData = new();
-        var fileElement = node as XmlElement;
+        if (node is not XmlElement fileElement)
+        {
+            // Documented above: a node that is not an element yields defaults. ReadFilterTabs walks every
+            // child of filterTabs, so a comment or text node in a hand-edited .lxp arrives here.
+            return persistenceData;
+        }
+
         persistenceData.BookmarkList = ReadBookmarks(fileElement);
         persistenceData.RowHeightList = ReadRowHeightList(fileElement);
         ReadOptions(fileElement, persistenceData);
@@ -447,17 +453,59 @@ private static SortedList<int, RowHeightEntry> ReadRowHeightList (XmlElement sta
     }
 
     /// <summary>
-    /// Reads configuration options from the specified XML element and populates the provided <see
-    /// cref="PersistenceData"/> object with the extracted settings.
+    /// Reads the <c>options</c> section of a <c>file</c> element, plus the <c>tab</c>, <c>columnizer</c> and
+    /// <c>highlightGroup</c> elements that sit beside it, into the provided <see cref="PersistenceData"/>.
     /// </summary>
-    /// <remarks>This method processes various configuration options such as multi-file settings, current line
-    /// information, filter visibility, and more. It expects the XML structure to contain specific nodes and attributes
-    /// that define these settings. If certain attributes are missing or invalid, default values are applied.</remarks>
-    /// <param name="startNode">The XML element containing the configuration options to be read.</param>
+    /// <remarks>
+    /// The <c>options</c> section is delegated to <see cref="ReadOptionsNode"/> and is optional: a hand-edited
+    /// or very old <c>.lxp</c> may omit it, in which case every setting it carries keeps its
+    /// <see cref="PersistenceData"/> default. The three sibling elements are read either way, so a missing
+    /// <c>options</c> section does not cost the user their tab name, columnizer or highlight group.
+    /// </remarks>
+    /// <param name="startNode">The <c>file</c> element to read from.</param>
     /// <param name="persistenceData">The <see cref="PersistenceData"/> object to populate with the settings extracted from the XML element.</param>
     private static void ReadOptions (XmlElement startNode, PersistenceData persistenceData)
     {
         XmlNode optionsNode = startNode.SelectSingleNode("options");
+        if (optionsNode != null)
+        {
+            ReadOptionsNode(optionsNode, persistenceData);
+        }
+
+        // tab, columnizer and highlightGroup are siblings of options, not children, so they are read
+        // even when a hand-edited or very old .lxp carries no options section at all.
+        XmlNode tabNode = startNode.SelectSingleNode("tab");
+        if (tabNode != null)
+        {
+            persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name");
+        }
+
+        XmlNode columnizerNode = startNode.SelectSingleNode("columnizer");
+        if (columnizerNode != null)
+        {
+            persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name");
+        }
+
+        XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup");
+        if (highlightGroupNode != null)
+        {
+            persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name");
+        }
+    }
+
+    /// <summary>
+    /// Reads the settings held under the <c>options</c> element into the given <see cref="PersistenceData"/>.
+    /// </summary>
+    /// <remarks>
+    /// Boolean flags are written unconditionally, so an absent element or attribute reads as <c>false</c>.
+    /// The remaining settings keep the value they already have. Not calling this at all — which is what
+    /// <see cref="ReadOptions"/> does for a <c>file</c> element without an <c>options</c> child — therefore
+    /// leaves the whole group at its <see cref="PersistenceData"/> default, <c>FollowTail</c> included.
+    /// </remarks>
+    /// <param name="optionsNode">The <c>options</c> element. Must not be <c>null</c>.</param>
+    /// <param name="persistenceData">The <see cref="PersistenceData"/> object to populate.</param>
+    private static void ReadOptionsNode (XmlNode optionsNode, PersistenceData persistenceData)
+    {
         var value = GetOptionsAttribute(optionsNode, "multifile", "enabled");
         persistenceData.MultiFile = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase);
         persistenceData.MultiFilePattern = GetOptionsAttribute(optionsNode, "multifile", "pattern");
@@ -530,24 +578,6 @@ FormatException or
 
         value = GetOptionsAttribute(optionsNode, "filterSaveList", "visible");
         persistenceData.FilterSaveListVisible = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase);
-
-        XmlNode tabNode = startNode.SelectSingleNode("tab");
-        if (tabNode != null)
-        {
-            persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name");
-        }
-
-        XmlNode columnizerNode = startNode.SelectSingleNode("columnizer");
-        if (columnizerNode != null)
-        {
-            persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name");
-        }
-
-        XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup");
-        if (highlightGroupNode != null)
-        {
-            persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name");
-        }
     }
 
     /// <summary>
@@ -599,8 +629,19 @@ private static string GetOptionsAttribute (XmlNode optionsNode, string elementNa
     /// when the file cannot be read or parsed (XML/IO/security related issues are logged).
     /// </returns>
     /// <remarks>
-    /// Only XML format is attempted. Any <see cref="XmlException"/>, <see cref="UnauthorizedAccessException"/>,
-    /// or <see cref="IOException"/> is caught and logged; in these cases <c>null</c> is returned.
+    /// Only XML format is attempted. Unreadable files (<see cref="UnauthorizedAccessException"/>,
+    /// <see cref="IOException"/>, <see cref="FileNotFoundException"/>) and unparseable content are caught
+    /// and logged; in these cases <c>null</c> is returned.
+    /// <para>
+    /// "Unparseable" covers more than malformed markup: this is a read-only fallback for a format LogExpert
+    /// no longer writes, so the readers below assume a well-formed document and let a hand-edited or truncated
+    /// one throw <see cref="FormatException"/> (unparseable numbers, invalid Base64),
+    /// <see cref="OverflowException"/> (numbers too large for their field), <see cref="ArgumentException"/>
+    /// (missing attributes, duplicate bookmark lines, a filter tab with no filter) or
+    /// <see cref="NullReferenceException"/> (attribute iteration over a comment or text node that stands where
+    /// an element was expected). The caller reaches this synchronously from the file-open path, so every one of
+    /// those has to degrade to "no persistence data" rather than escape.
+    /// </para>
     /// </remarks>
     public static PersistenceData Load (string fileName)
     {
@@ -611,7 +652,11 @@ public static PersistenceData Load (string fileName)
         catch (Exception xmlParsingException) when (xmlParsingException is XmlException or
                                                                            UnauthorizedAccessException or
                                                                            IOException or
-                                                                           FileNotFoundException)
+                                                                           FileNotFoundException or
+                                                                           FormatException or
+                                                                           OverflowException or
+                                                                           ArgumentException or
+                                                                           NullReferenceException)
         {
             _logger.Error(xmlParsingException, $"Error loading persistence data from {fileName}, unknown format, parsing xml or json was not possible");
             return null;

src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs

@@ -0,0 +1,144 @@
+using LogExpert.Core.Classes.Persister;
+
+namespace LogExpert.Persister.Tests;
+
+/// <summary>
+/// The deprecated XML .lxp format is a read-only fallback: it is only reached when a session file fails
+/// to parse as JSON, and that happens on the file-open path, synchronously, with no handler above it.
+/// A .lxp that is hand-edited, truncated, or written by a very old version must therefore degrade to
+/// "no persistence data" instead of throwing into the open.
+/// </summary>
+[TestFixture]
+#pragma warning disable CS0618 // PersisterXML is the deprecated fallback format, and still loads old .lxp files.
+public class PersisterXmlMalformedTests
+{
+    private string _lxpFile = null!;
+
+    [SetUp]
+    public void Setup ()
+    {
+        _lxpFile = Path.Join(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp");
+    }
+
+    [TearDown]
+    public void Cleanup ()
+    {
+        if (File.Exists(_lxpFile))
+        {
+            File.Delete(_lxpFile);
+        }
+    }
+
+    [Test]
+    public void Load_FileElementWithoutOptions_ReturnsDefaults ()
+    {
+        WriteLxp("""<file fileName="test.log" lineCount="1" />""");
+
+        var data = PersisterXML.Load(_lxpFile);
+
+        Assert.That(data, Is.Not.Null, "a <file> without <options> must still load");
+        Assert.Multiple(() =>
+        {
+            Assert.That(data.FileName, Is.EqualTo("test.log"));
+            Assert.That(data.LineCount, Is.EqualTo(1));
+            Assert.That(data.MultiFile, Is.False);
+            Assert.That(data.MultiFilePattern, Is.Null);
+            Assert.That(data.MultiFileMaxDays, Is.Zero);
+            Assert.That(data.FilterVisible, Is.False);
+
+            // A missing <options> means "nothing was persisted", so every option keeps its
+            // PersistenceData default — including the ones whose default is not the zero value.
+            Assert.That(data.CurrentLine, Is.EqualTo(-1));
+            Assert.That(data.FollowTail, Is.True);
+        });
+    }
+
+    [Test]
+    public void Load_FileElementWithoutOptions_StillReadsSiblingsOfOptions ()
+    {
+        // tab, columnizer and highlightGroup are children of <file>, not of <options>. A missing
+        // <options> must not cost the user their columnizer and highlight group.
+        WriteLxp("""
+                 <file fileName="test.log" lineCount="1">
+                   <tab name="My Tab" />
+                   <columnizer name="CSV Columnizer" />
+                   <highlightGroup name="My Group" />
+                 </file>
+                 """);
+
+        var data = PersisterXML.Load(_lxpFile);
+
+        Assert.That(data, Is.Not.Null);
+        Assert.Multiple(() =>
+        {
+            Assert.That(data.TabName, Is.EqualTo("My Tab"));
+            Assert.That(data.ColumnizerName, Is.EqualTo("CSV Columnizer"));
+            Assert.That(data.HighlightGroupName, Is.EqualTo("My Group"));
+        });
+    }
+
+    /// <summary>
+    /// Load documents "null when the file cannot be read or parsed", and malformed content counts, not
+    /// just malformed markup. One case per exception the readers let escape, so the widened catch in
+    /// Load is pinned rather than merely asserted in prose.
+    /// </summary>
+    [Test]
+    [TestCase("""<file fileName="test.log" lineCount="not-a-number"><options /></file>""", "FormatException")]
+    [TestCase("""<file fileName="test.log" lineCount="99999999999999"><options /></file>""", "OverflowException")]
+    [TestCase("""<file fileName="test.log" lineCount="1"><options /><bookmarks><bookmark line="5"><posX>1</posX><posY>2</posY></bookmark><bookmark line="5"><posX>3</posX><posY>4</posY></bookmark></bookmarks></file>""", "ArgumentException")]
+    [TestCase("""<file fileName="test.log" lineCount="1"><options /><bookmarks><!-- hand-edited remark --></bookmarks></file>""", "NullReferenceException")]
+    public void Load_MalformedContent_ReturnsNull (string fileElement, string escapingException)
+    {
+        WriteLxp(fileElement);
+
+        Assert.That(PersisterXML.Load(_lxpFile), Is.Null, $"{escapingException} escaped Load");
+    }
+
+    [Test]
+    public void Load_FilterTabsWithNonElementChild_SkipsIt ()
+    {
+        WriteLxp("""
+                 <file fileName="test.log" lineCount="1">
+                   <options />
+                   <filterTabs><!-- hand-edited remark --></filterTabs>
+                 </file>
+                 """);
+
+        var data = PersisterXML.Load(_lxpFile);
+
+        // ReadPersistenceDataFromNode documents defaults for a node that is not an element, so the
+        // remark costs the reader nothing beyond the tab it is standing in for.
+        Assert.That(data, Is.Not.Null);
+        Assert.Multiple(() =>
+        {
+            Assert.That(data.FileName, Is.EqualTo("test.log"));
+            Assert.That(data.FilterTabDataList, Is.Empty);
+        });
+    }
+
+    [Test]
+    public void PersisterLoad_FileElementWithoutOptions_DoesNotThrowOnTheFileOpenPath ()
+    {
+        // Persister.Load tries JSON first and only falls back to XML from inside the catch block, so
+        // anything PersisterXML throws replaces the original exception and escapes into AddFileTab.
+        WriteLxp("""<file fileName="test.log" lineCount="1" />""");
+
+        var data = Core.Classes.Persister.Persister.Load(_lxpFile);
+
+        Assert.That(data, Is.Not.Null);
+        Assert.That(data.FileName, Is.EqualTo("test.log"));
+    }
+
+    private void WriteLxp (string fileElement)
+    {
+        File.WriteAllText(
+            _lxpFile,
+            $"""
+            <?xml version="1.0" encoding="utf-8"?>
+            <logexpert>
+              {fileElement}
+            </logexpert>
+            """);
+    }
+}
+#pragma warning restore CS0618

src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs

@@ -73,7 +73,6 @@ public void Load_UnresolvableEncodingName_FallsBackToDefault ()
 
     private void WriteLxp (string encodingElement)
     {
-        // <options /> has to be present: PersisterXML.ReadOptions dereferences it unconditionally.
         File.WriteAllText(
             _lxpFile,
             $"""