LT-14056: Crash with Undo then Redo deleted Lexical Relations#383
Conversation
Add null cache guards. The redo is deleting a lexical relation that was just "undeleted" via undo. Tries to restore refs when the object has already been deleted.
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on aror92).
src/SIL.LCModel/DomainImpl/CmObject.cs line 2634 at r1 (raw file):
if (Cache == null) { Debug.Fail("Restoring incoming refs on outgoing refs for an object already deleted");
Why do we want the Debug.Fail() messages (both this one and the other)? To me they imply that there is something wrong that needs to be investigated. If the correct solution is to just return (as you are doing), then we probably shouldn't be alerting the developers.
aror92
left a comment
There was a problem hiding this comment.
@aror92 made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on mark-sil).
src/SIL.LCModel/DomainImpl/CmObject.cs line 2634 at r1 (raw file):
Previously, mark-sil (Mark Kidder) wrote…
Why do we want the Debug.Fail() messages (both this one and the other)? To me they imply that there is something wrong that needs to be investigated. If the correct solution is to just return (as you are doing), then we probably shouldn't be alerting the developers.
The Debug.Fail was an addition recommended by Devin/LcmDeepWiki. I was torn on it, but Devin pointed out that we already have one guard for the same reason in ClearIncomingRefsOnOutgoingRefsInternal, and in that case, it includes the comment "SHOULD never be called on already deleted object" and uses a Debug.Fail message for that reason.
The same logic would apply to both of these guards, so I included the Debug.Fail messages. (i.e. Cache == null means undo/redo is operating on an already deleted object, which we appear to claim should never happen by design.) I'm okay removing the Debug.Fails, though, if we're sure we don't want them.
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on aror92).
Add null cache guards. The redo is deleting a lexical relation that was just "undeleted" via undo. Tries to restore refs when the object has already been deleted.
This change is