Fix delete-while-iterating in delta ensemble case removal - #14518
Merged
Conversation
…semble Removing a source case makes a delta ensemble recreate and delete its derived cases. Those cases are part of the list being removed, leaving dangling pointers that crash in PdmObjectHandle::prepareForDelete(). Use guarded pointers and return only the surviving cases.
Delta ensembles were located by scanning the summary case main collection for objects referring to a given ensemble, and the dependent ensembles were visited by unguarded recursion. A dependency cycle, which is constructible through the UI, made that recursion run forever, and an ensemble used as both source 1 and source 2 was reported twice. Add dependentDeltaEnsembles(), deltaEnsemblesInUpdateOrder() and wouldCreateDependencyCycle() to RimSummaryEnsembleTools. The traversal uses the PDM back references, deduplicates, is iterative with visited and on-path sets, and returns the delta ensembles in topological order so a delta ensemble is always visited before the delta ensembles using it as a source. Back edges are logged instead of traversed. Reimplement updateDependentDeltaEnsembles on top of the new traversal and replace RimDeltaSummaryEnsemble::findReferringEnsembles() with dependentDeltaEnsembles() at its four call sites. Back references also find a delta ensemble that is detached from the project tree, which the previous ancestor scan did not.
Derived cases were pooled through an m_inUse flag on RimDeltaSummaryCase. Every regeneration marked all cases not in use, which also severed their source references and cleared their caches, then handed them back out one by one and deleted whatever was left over. The flag conflated pool bookkeeping with owning the source references, allSummaryCases() hid the not-in-use cases from the rest of the project, and cases taken from the pool were pushed straight into m_cases without connecting nameChanged, so a renamed source case never propagated to the derived case. Replace the pooling with desiredSourceCasePairs(), a pure computation of the source case pairs the ensemble should have, and rebuildDerivedCases(), which diffs that against the existing derived cases keyed on the source case pointer pair. Matching cases are reused, missing ones are created, and surplus ones are detached and returned to the caller instead of being deleted in place. Keying on the pointer pair makes the rebuild idempotent, also right after project load where the derived cases arrive from XML with their sources already resolved. Add the protected RimSummaryEnsemble::addCaseWithoutDependencyUpdate(), used both by addCase() and by the rebuild, so a derived case gets nameChanged connected without triggering the dependent-ensemble notification that the rebuild is already performing itself. Remove setAllCasesNotInUse(), firstCaseNotInUse(), deleteCasesNoInUse(), RimDeltaSummaryCase::setInUse()/isInUse() and the m_inUse field, and add clearSourceCases() for the one thing setInUse(false) was actually needed for. Old project files keep loading, unknown XML keywords are skipped. The activeOnly parameter of allDerivedCases() is gone and RimDeltaSummaryEnsemble no longer overrides allSummaryCases().
Closing all summary cases while a delta ensemble is present crashed with a use-after-free. RicCloseSummaryCaseFeature::deleteSummaryCases holds a case list across removeCases and deletes it afterwards, while removeCases made the delta ensemble rebuild and destroy derived cases that are themselves part of that list. The previous fix rewrote the caller list with the surviving cases, which stopped the crash but left the hazard in place for any other caller holding a case list across a removal. Add RimSummaryCaseUpdateBatch, a plain scope object that is ambient for the duration of its scope. Removal now only detaches, and hands the detached cases to the batch. The outermost scope flushes, regenerating the dirty delta ensembles in dependency order first so a chained delta ensemble sees the final state of its source, then destroying the orphans with caf::PdmObjectHandleTools::deleteObjects. A nested batch contributes to the outermost one and never flushes. Orphans are held as guarded pointers, so a case the caller destroyed itself is skipped instead of being destroyed twice. Both contribution points fall back to immediate execution when no batch is active, so call sites that do not open one keep behaving as before. Open a batch in RimSummaryCaseMainCollection::removeCases and in RicCloseSummaryCaseFeature::deleteSummaryCases, which is the outermost of the two and therefore keeps the detached cases alive across its own deleteObjects call. removeCases no longer rewrites the caller list, so it now takes it by const reference.
RimDeltaSummaryEnsemble-Test.cpp and RimSummaryCaseMainCollection-Test.cpp each defined an identical createMockCase() in an anonymous namespace. A unity build concatenates the two translation units, which merges the two anonymous namespaces into one and makes the second definition a redefinition, breaking the build with C2084. Move the factory to RimMockSummaryCase.h, the header both tests already include for the mock case itself, and drop both local copies.
The project is a global object shared by all tests, and the MSW export tests loaded a project without closing it. The two summary cases of that project stayed in the summary case main collection, and made RimSummaryCaseMainCollection.RemoveCases_NoDanglingInCallerVector fail on Windows only. Google Test runs the test suites in link order, and the MSW suite runs before the summary suite on Windows and after it on Linux. Add a test event listener reporting a failure for the test leaving cases, ensembles, well paths or views behind in the project. The project is closed as well, so the tests running after the offending one are unaffected.
magnesj
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14517.
Removes the delete-while-iterating hazard behind the crash in #14423, rather than working around it at one call site as #14424 does. Three commits, each independently testable.
Detach never destroys
RimDeltaSummaryEnsembledeleted its derived cases inline, from inside a loop the caller was still walking.rebuildDerivedCases()replaces them_inUsepooling: it diffs the derived cases against the desired source pairs, keyed on the(case1, case2)pointer pair, reuses matches, creates what is missing, and returns the surplus instead of deleting it. Keying on the pointer pair makes the rebuild idempotent, including right after project load where derived cases arrive from XML with their sources already resolved.The
m_inUseflag conflated pool bookkeeping with owning the source references and the value cache. It also hid the not-in-use cases fromallSummaryCases(), which is precisely why the dangling objects were invisible to the rest of the project.setInUse,isInUse,m_inUse,setAllCasesNotInUse,firstCaseNotInUseanddeleteCasesNoInUseare gone. Old project files still load, unknown XML keywords are skipped.Destruction happens at a batch flush
RimSummaryCaseUpdateBatchis a plain scope object, ambient for its scope. Detached cases are handed to it and destroyed when the outermost scope ends, after the dirty delta ensembles have regenerated in dependency order, so a chained delta ensemble sees the final state of its source before anything is freed. A nested batch contributes to the outermost one and never flushes. Orphans are held ascaf::PdmPointer, so a case the caller destroyed itself is skipped rather than destroyed twice. With no batch active both contribution points fall back to immediate execution, so call sites that do not open one keep behaving as before.RicCloseSummaryCaseFeature::deleteSummaryCasesopens the outermost batch. It has to be there rather than inremoveCases, because it callscaf::PdmObjectHandleTools::deleteObjects( cases )afterremoveCasesreturns — a batch ending withremoveCaseswould still free the orphans while that list refers to them.removeCasesopens one of its own for callers that do not, no longer rewrites the caller's list, and takes it by const reference.Dependency traversal by PDM back-reference
findReferringEnsembles()scannedsummaryEnsembles()and recursed with no visited set, so a dependency cycle ran forever, and an ensemble used as both source 1 and source 2 was reported twice. Replaced bydependentDeltaEnsembles(),deltaEnsemblesInUpdateOrder()andwouldCreateDependencyCycle()inRimSummaryEnsembleTools— deduplicated, iterative, topologically ordered, back edges logged rather than traversed. Back references also reach a delta ensemble detached from the project tree, which the ancestor scan did not.Relation to #14424
The first commit here is a cherry-pick of
e16238f331from #14424, so the regression test it introduced is present; the last commit removes the band-aid itself. The net diff againstdevtherefore keeps the test and drops thecaf::PdmPointerworkaround. #14424 can be closed in favour of this. If it merges first instead, the cherry-picked commit drops out on rebase and nothing else changes.Verification
1033 unit tests pass, from 1028 on the base.
RemoveCases_NoDanglingInCallerVectorasserts that every entry the caller handed over is still alive inside the batch scope and destroyed exactly once after it exits. Removing just the batch line from that test reproduces the original signature:cases.size()6 againstaliveCount()4, followed bySEH exception with code 0xc0000005. New tests also cover dependency ordering over chained delta ensembles, cycle termination, back-reference deduplication, rebuild idempotency, and deferred destruction with and without batch nesting.Known follow-up, not in this PR
A dependency cycle stack-overflows in auto-name generation, independently of the traversal fixed here:
RimSummaryEnsemble::updateNameends incaseNameChanged.send(), which reachesRimSummaryCaseMainCollection::onCaseNameChangedand back intoupdateSummaryEnsembleNames(). It terminates only because names converge, and auto-generated names never converge when A's name derives from B's and B's from A's.DependencyOrder_CycleTerminatescloses its cycle by writing thePdmPtrFielddirectly for that reason. Cycle prevention insetEnsemble1()/setEnsemble2()is the fix.Staged plan and the remaining stages: magnesj#1031.