DeepWiki-found bugs

[imnasnainaec]

https://deepwiki.com/sillsdev/liblcm
https://deepwiki.com/search/find-bugs_563f89be-f25d-46f1-9d57-0afdf13b424c?mode=deep

Here are the bugs found across the codebase, ordered roughly by severity:

---

Bug 1: NullReferenceException in MoMorphData.ActiveParser setter

File: src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs

(L4218-4234)

				XmlDocument doc = new XmlDocument();
				XmlNode paramsNode = null;
				try
				{
					doc.LoadXml(ParserParameters);
					paramsNode = doc.SelectSingleNode("/ParserParameters");
				}
				catch
				{
					// Eat exception.
				}

				if (paramsNode == null)
				{
					paramsNode = doc.CreateElement("ParserParameters");
					doc.DocumentElement.AppendChild(paramsNode);
				}

When doc.LoadXml(ParserParameters) throws (e.g., ParserParameters is null or malformed XML), the exception is silently swallowed and paramsNode stays null. The code then calls doc.DocumentElement.AppendChild(paramsNode), but doc.DocumentElement is null on a freshly constructed XmlDocument, causing a NullReferenceException. The fix is doc.AppendChild(paramsNode).

---

Bug 2: Skipped run after removal in CleanChapterInBtPara

File: src/SIL.LCModel/DomainImpl/ScrTxtPara.cs

(L1751-1760)

		private void CleanChapterInBtPara(int wsAlt, BCVRef startRef, BCVRef endRef)
		{
			int iRun = 0;

			while (iRun < GetOrCreateBT().Translation.get_String(wsAlt).RunCount)
			{
				RemoveOutOfRangeChapterRun(iRun, wsAlt, startRef, endRef);
				iRun++;
			}
		}

iRun is unconditionally incremented even when RemoveOutOfRangeChapterRun returns true (meaning a run was deleted and all subsequent runs shifted down by one). The run that slides into position iRun is never examined. Two consecutive out-of-range chapter runs will cause the second one to be silently skipped.

---

Bug 3: Potential NullReferenceException in MergeSelectedPropertiesOfObject

File: src/SIL.LCModel/DomainImpl/CmObject.cs

(L1058-1060)

							var myAddMethod = myCurrentValue.GetType().GetMethod("Add");
							foreach (var input in ((ILcmVector)srcCurrentValue).Objects)
								myAddMethod.Invoke(myCurrentValue, new object[] { input });

myCurrentValue.GetType().GetMethod("Add") can return null if the runtime type doesn't expose a public Add method with the expected signature (e.g., due to explicit interface implementation). The result is used immediately without a null check, so myAddMethod.Invoke(...) will throw a NullReferenceException.

---

Bug 4: Silent data loss in CollectVars for sequence contexts

File: src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs

(L4157-4161)

				case PhSequenceContextTags.kClassId:
					var seqCtxt = (IPhSequenceContext)ctxt;
					foreach (var cur in seqCtxt.MembersRS)
						CollectVars(cur as IPhSimpleContextNC, featureConstrs);
					break;

In the PhSequenceContextTags.kClassId case, each member is cast with cur as IPhSimpleContextNC. If a member is not an IPhSimpleContextNC (e.g., it is a PhIterationContext or PhSequenceContext), the cast silently returns null, and the null-guard at the top of CollectVars returns early — those members' variables are never collected.

---

Bug 5: Managed object access from finalizer in UnitOfWorkService.Dispose

File: src/SIL.LCModel/Infrastructure/Impl/UnitOfWorkService.cs

(L215-223)

			if (fDisposing && !IsDisposed)
			{
				// dispose managed and unmanaged objects
				m_lock.Dispose();
				m_saveTimer.Dispose();
			}
			IsDisposed = true;
			m_logger?.AddBreadCrumb("UOW Disposed.");
		}

m_logger?.AddBreadCrumb("UOW Disposed.") is placed outside the if (fDisposing && !IsDisposed) guard. When called from the finalizer (fDisposing == false), this accesses the managed m_logger object, which may already have been garbage-collected. This violates the standard dispose pattern and can cause unpredictable behavior.

---

Bug 6: NullReferenceException in WsString.Equals and GetHashCode

File: src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs

(L2492-2507)

			public override bool Equals(object obj)
			{
				if (obj is WsString)
				{
					return (ws == ((WsString)obj).ws) && sVal.Equals(((WsString)obj).sVal);
				}
				else
				{
					return false;
				}
			}

			public override int GetHashCode()
			{
				return sVal.GetHashCode() + (ws % 100);
			}

WsString.Equals calls sVal.Equals(...) and GetHashCode calls sVal.GetHashCode() without guarding against sVal being null. If a WsString is constructed with a null string value, both methods will throw NullReferenceException.

---

Bug 7: Wrong exception type in IndexOfFirstUnusedId

File: src/SIL.LCModel/DomainServices/ITextUtils.cs

(L875-880)

		static protected int IndexOfFirstUnusedId(List<int> ids)
		{
			if (ids == null)
				throw new NullReferenceException("ids");
			return IndexOfFirstUnusedId(ids.ToArray());
		}

The method explicitly throws new NullReferenceException("ids") for a null argument. This should be ArgumentNullException, which is the standard .NET convention for null parameter validation. NullReferenceException is reserved for unintentional null dereferences.

---

Bug 8: Potential deadlock in FileTransactionLogger finalizer

File: src/SIL.LCModel/FileTransactionLogger.cs

(L41-56)

	   protected virtual void Dispose(bool disposing)
		{
			Debug.WriteLineIf(!disposing, "****** Missing Dispose() call for " + GetType() + ". *******");
			lock (m_lock)
			{
				if (!m_disposed)
				{
					try { m_lastWrite?.GetAwaiter().GetResult(); }
					catch { /* best-effort: don't let a failed write prevent disposal */ }
					m_stream?.Flush();
					m_stream?.Dispose();
					m_stream = null;
					m_disposed = true;
				}
			}
		}

The finalizer calls Dispose(false), which acquires m_lock and then calls m_lastWrite?.GetAwaiter().GetResult(). Blocking on a Task from a finalizer thread is dangerous: if the continuation was scheduled to run on the finalizer thread (via ExecuteSynchronously), it cannot run while the finalizer thread is blocked waiting for it — a deadlock. The finalizer path should skip the GetResult() call entirely.

[imnasnainaec]

Claude Fable 5 review

I verified each report against current master. All eight cite real code accurately, but they vary widely in practical severity. Per-bug assessment:

Bug 1 (ActiveParser setter NRE) — confirmed. If LoadXml throws (null/empty/malformed ParserParameters), doc has no root and doc.DocumentElement.AppendChild(paramsNode) throws NRE. The getter deliberately tolerates malformed ParserParameters, so the setter should too. One caveat on the suggested fix: doc.AppendChild(paramsNode) is only correct in the load-failed case. There is a second path to paramsNode == null — ParserParameters is valid XML with a different root element — where doc.AppendChild would throw because the document already has a root. A robust fix resets the document (doc = new XmlDocument()) before appending when /ParserParameters isn't found.

Bug 2 (CleanChapterInBtPara skips a run) — confirmed. The increment should be conditional: if (!RemoveOutOfRangeChapterRun(...)) iRun++;. Mitigating factors: two adjacent chapter-number runs with identical props would already be a single run, so triggering this requires adjacent chapter runs with differing props (e.g. different writing systems), and this is in the legacy Scripture/TE domain. Low practical impact, but the fix is one line.

Bug 3 (GetMethod("Add") null) — not reachable in practice; would skip. The values hitting these cases are always the LCM vector implementations (LcmSet<T>/LcmList<T> and subclasses in DomainImpl/Vectors.cs), all of which expose a single public Add(T) (Vectors.cs:306, 646, 1431), so GetMethod("Add") cannot return null with current types. A null guard with a descriptive exception would only marginally improve an already-impossible failure mode.

Bug 4 (CollectVars sequence members) — confirmed; the most likely of these to affect real behavior. Feature constraints are silently missed when a sequence context contains an iteration context wrapping a natural-class context, which affects alpha-variable collection for the parser. The clean fix is to recurse with the general overload — CollectVars(cur, featureConstrs) — since the switch already handles iteration/sequence/NC. Note the same pattern exists in the CollectVars(IEnumerable<IPhContextOrVar>, ...) overload just above (OverridesLing_MoClasses.cs:4129-4136), which also only handles PhSimpleContextNC members; both sites should be fixed together.

Bug 5 (UOW dispose breadcrumb outside guard) — confirmed but minor. Two corrections to the writeup: the UnitOfWorkService finalizer only exists in DEBUG builds, and "may already have been garbage-collected" is inaccurate — m_logger is still reachable, so it can't be collected; it may however already have been finalized (and FileTransactionLogger.AddBreadCrumb early-returns once disposed, which limits the damage). Still a dispose-pattern violation in recently added code (#360); moving the breadcrumb inside the if (fDisposing && !IsDisposed) block is the right fix.

Bug 6 (WsString null sVal) — latent only. I checked every new WsString construction site in XmlImportData.cs: sVal is never null at the point the struct is hashed or compared (the two default-constructed instances at L2791/2804 are only used inside branches that assign sVal first). WsStringIgnoreCaseComparer (L2516/2521) has the same theoretical issue. Fine to harden opportunistically; no reachable crash today.

Bug 7 (NullReferenceException vs ArgumentNullException) — correct but trivial. Pure convention fix; safe to make since no reasonable caller catches NullReferenceException deliberately.

Bug 8 (finalizer blocking on task) — right conclusion, wrong mechanism. The claimed deadlock (ExecuteSynchronously continuation "scheduled on the finalizer thread") isn't how the TPL works: the continuation runs inline on whichever thread completes the antecedent (an I/O completion thread), not on the blocked finalizer thread. The real problems are that the finalizer touches managed state (m_lastWrite, m_stream — which may already be finalized) and blocks the finalizer thread, which can hang finalization or process shutdown if a write never completes. The recommended fix stands: Dispose(false) should do nothing beyond marking disposed — the class owns no unmanaged resources directly and FileStream has its own safe-handle finalization — or the finalizer could be removed entirely.

Summary

• Worth fixing: 1, 2, 4 (real logic bugs; 4 is the only one plausibly affecting normal FLEx use), plus 5 and 8 together as a dispose-pattern cleanup in the recently added transaction-logging code, and 7 as a trivial drive-by.
• Skip: 3 (unreachable with current types); 6 (latent, harden only if touching that code anyway).

Review performed with Claude Code; each finding checked against the cited source.

[imnasnainaec]

Implementation summary for the six recommended fixes:

Bug #	Fix size	Tests to add?	Files touched	Breaking?
1	~5 lines (reset doc when /ParserParameters not found)	Yes — setter with null/empty/malformed and wrong-root ParserParameters (fits in LingTests.cs)	OverridesLing_MoClasses.cs + test file	No — turns a crash into the intended behavior
2	1 line (if (!RemoveOutOfRangeChapterRun(...)) iRun++;)	Yes, if practical — BT para with adjacent chapter runs in different writing systems (ScrTxtParaTests.cs exists)	ScrTxtPara.cs + test file	No
4	~2 lines across the two CollectVars sites (recurse via the general overload)	Yes — iteration context nested inside a sequence context, assert constraints collected (fits in LingTests.cs)	OverridesLing_MoClasses.cs + test file	No — collecting the missed constraints is the fix, but worth a parser regression check in FLEx
5	1 line (move breadcrumb inside the dispose guard)	No — DEBUG-only finalizer path, not meaningfully unit-testable	UnitOfWorkService.cs	No
7	1 line (ArgumentNullException)	No	ITextUtils.cs	Technically changes the thrown exception type on a protected member; no realistic caller depends on catching NullReferenceException
8	~3 lines (early-return from Dispose(false), or drop the finalizer)	No — finalizer timing isn't reliably testable; existing FileTransactionLoggerTests.cs covers the normal dispose path and should still pass	FileTransactionLogger.cs	No

Bugs 1, 2, and 4 each want a regression test; 5, 7, and 8 are safe as untested one-liners. All six could go in a single PR, or split 5+8 into a separate dispose-pattern cleanup since they touch the transaction-logging code added in #360.