From 3995592cdf304335132305e27c40cbb0b1ac46e3 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Wed, 10 Jan 2018 19:21:59 -0500 Subject: [PATCH] Debugger: Fix #13939, more work on #13800. - Fix various cases where OpenHashTables weren't being cleared properly. - Fix various reference counting errors. - Simplify FileManager reference handling. - Fix bug in LocatableDirectory where the directory named '/' would have its name returned as empty. This would lead to failed lookups for entries already in the table, and ultimately corrupted the hash table when deleting unused entries, leading to #13939. This was previously never noticed due to the entries not being freed properly. - AbbreviationTable wasn't clearing its entries. --- .../gui/model/VariablesViewState.cpp | 1 + .../gui/team_window/SourceView.cpp | 2 +- .../gui/team_window/VariablesView.cpp | 15 ++++++++-- .../value_handlers/IntegerValueHandler.cpp | 5 ++-- .../debug_info/DwarfImageDebugInfo.cpp | 2 +- .../debug_managers/ValueNodeManager.cpp | 3 ++ src/kits/debugger/dwarf/AbbreviationTable.cpp | 6 ++++ src/kits/debugger/files/FileManager.cpp | 28 +++++-------------- src/kits/debugger/files/FileManager.h | 2 -- .../debugger/files/LocatableDirectory.cpp | 3 ++ src/kits/debugger/files/LocatableEntry.cpp | 1 + src/kits/debugger/model/StackFrame.cpp | 6 ++++ src/kits/debugger/settings/TeamSettings.cpp | 2 ++ .../debugger/value/ValueNodeContainer.cpp | 2 ++ .../value/value_nodes/BListValueNode.cpp | 6 ++-- .../value/value_nodes/BMessageValueNode.cpp | 7 +++-- 16 files changed, 56 insertions(+), 35 deletions(-) diff --git a/src/apps/debugger/user_interface/gui/model/VariablesViewState.cpp b/src/apps/debugger/user_interface/gui/model/VariablesViewState.cpp index 081f7591f6..e6979f262b 100644 --- a/src/apps/debugger/user_interface/gui/model/VariablesViewState.cpp +++ b/src/apps/debugger/user_interface/gui/model/VariablesViewState.cpp @@ -261,4 +261,5 @@ VariablesViewState::_Cleanup() } SetValues(NULL); + SetExpressionValues(NULL); } diff --git a/src/apps/debugger/user_interface/gui/team_window/SourceView.cpp b/src/apps/debugger/user_interface/gui/team_window/SourceView.cpp index ee30aae585..2d964fbf49 100644 --- a/src/apps/debugger/user_interface/gui/team_window/SourceView.cpp +++ b/src/apps/debugger/user_interface/gui/team_window/SourceView.cpp @@ -2175,7 +2175,7 @@ SourceView::SetStackTrace(StackTrace* stackTrace, Thread* activeThread) { TRACE_GUI("SourceView::SetStackTrace(%p)\n", stackTrace); - if (stackTrace == fStackTrace) + if (stackTrace == fStackTrace && activeThread == fActiveThread) return; if (fActiveThread != NULL) diff --git a/src/apps/debugger/user_interface/gui/team_window/VariablesView.cpp b/src/apps/debugger/user_interface/gui/team_window/VariablesView.cpp index 1fc7b75546..a30bba3d0f 100644 --- a/src/apps/debugger/user_interface/gui/team_window/VariablesView.cpp +++ b/src/apps/debugger/user_interface/gui/team_window/VariablesView.cpp @@ -1324,6 +1324,7 @@ VariablesView::VariableTableModel::ValueNodeValueChanged(ValueNode* valueNode) if (error != B_OK) return; + BReference rendererReference(renderer, true); // set value/handler/renderer modelNode->SetValue(value); modelNode->SetValueHandler(valueHandler); @@ -1337,8 +1338,6 @@ VariablesView::VariableTableModel::ValueNodeValueChanged(ValueNode* valueNode) settings->RestoreValues(modelNode->GetLastRendererSettings()); } - - // notify table model listeners NotifyNodeChanged(modelNode); } @@ -1803,6 +1802,17 @@ VariablesView::~VariablesView() if (fTemporaryExpression != NULL) fTemporaryExpression->ReleaseReference(); + + if (fExpressions != NULL) { + ExpressionInfoEntry* entry = fExpressions->Clear(); + while (entry != NULL) { + ExpressionInfoEntry* next = entry->next; + delete entry; + entry = next; + } + } + + delete fExpressions; } @@ -2246,6 +2256,7 @@ VariablesView::MessageReceived(BMessage* message) { ModelNode* node; if (message->FindPointer("node", (void**)&node) == B_OK) { + BReference nodeReference(node, true); TreeTablePath path; if (fVariableTableModel->GetTreePath(node, path)) { FunctionID* functionID = fStackFrame->Function() diff --git a/src/apps/debugger/user_interface/gui/value/value_handlers/IntegerValueHandler.cpp b/src/apps/debugger/user_interface/gui/value/value_handlers/IntegerValueHandler.cpp index 1e2a3fb270..ef96164cee 100644 --- a/src/apps/debugger/user_interface/gui/value/value_handlers/IntegerValueHandler.cpp +++ b/src/apps/debugger/user_interface/gui/value/value_handlers/IntegerValueHandler.cpp @@ -343,10 +343,9 @@ IntegerValueHandler::AddIntegerFormatOption(OptionsSettingImpl* setting, const char* id, const char* name, integer_format format) { FormatOption* option = new(std::nothrow) FormatOption(id, name, format); - if (option == NULL || !setting->AddOption(option)) { - delete option; + BReference optionReference(option, true); + if (option == NULL || !setting->AddOption(option)) return B_NO_MEMORY; - } return B_OK; } diff --git a/src/kits/debugger/debug_info/DwarfImageDebugInfo.cpp b/src/kits/debugger/debug_info/DwarfImageDebugInfo.cpp index c38e6b83bb..1d8f420887 100644 --- a/src/kits/debugger/debug_info/DwarfImageDebugInfo.cpp +++ b/src/kits/debugger/debug_info/DwarfImageDebugInfo.cpp @@ -1111,7 +1111,7 @@ DwarfImageDebugInfo::_GetSourceFileIndex(CompilationUnit* unit, for (int32 i = 0; const char* fileName = unit->FileAt(i, &directory); i++) { LocatableFile* file = fFileManager->GetSourceFile(directory, fileName); if (file != NULL) { - file->ReleaseReference(); + BReference fileReference(file, true); if (file == sourceFile) { return i + 1; // indices are one-based diff --git a/src/kits/debugger/debug_managers/ValueNodeManager.cpp b/src/kits/debugger/debug_managers/ValueNodeManager.cpp index d59cf6df23..2fb4367f45 100644 --- a/src/kits/debugger/debug_managers/ValueNodeManager.cpp +++ b/src/kits/debugger/debug_managers/ValueNodeManager.cpp @@ -52,6 +52,9 @@ ValueNodeManager::SetStackFrame(Thread* thread, fStackFrame = stackFrame; fThread = thread; + if (fStackFrame == NULL) + return B_OK; + fContainer = new(std::nothrow) ValueNodeContainer; if (fContainer == NULL) return B_NO_MEMORY; diff --git a/src/kits/debugger/dwarf/AbbreviationTable.cpp b/src/kits/debugger/dwarf/AbbreviationTable.cpp index 337e10c26f..7dce4daa52 100644 --- a/src/kits/debugger/dwarf/AbbreviationTable.cpp +++ b/src/kits/debugger/dwarf/AbbreviationTable.cpp @@ -21,6 +21,12 @@ AbbreviationTable::AbbreviationTable(off_t offset) AbbreviationTable::~AbbreviationTable() { + AbbreviationTableEntry* entry = fEntryTable.Clear(true); + while (entry != NULL) { + AbbreviationTableEntry* next = entry->next; + delete entry; + entry = next; + } } diff --git a/src/kits/debugger/files/FileManager.cpp b/src/kits/debugger/files/FileManager.cpp index 9615b337ef..26f3ab5e9d 100644 --- a/src/kits/debugger/files/FileManager.cpp +++ b/src/kits/debugger/files/FileManager.cpp @@ -124,9 +124,6 @@ public: entry->ReleaseReference(); entry = next; } - - while ((entry = fDeadEntries.RemoveHead()) != NULL) - entry->ReleaseReference(); } status_t Init() @@ -202,21 +199,10 @@ private: AutoLocker lock(fManager); if (fEntries.Lookup(EntryPath(entry)) == entry) fEntries.Remove(entry); - else { - DeadEntryList::Iterator iterator = fDeadEntries.GetIterator(); - while (iterator.HasNext()) { - if (iterator.Next() == entry) { - fDeadEntries.Remove(entry); - break; - } - } - } LocatableDirectory* parent = entry->Parent(); if (parent != NULL) parent->RemoveEntry(entry); - - delete entry; } bool _LocateDirectory(LocatableDirectory* directory, @@ -306,23 +292,23 @@ private: LocatableFile* _GetFile(const BString& directoryPath, const BString& name) { + BString normalizedDirPath; + _NormalizePath(directoryPath, normalizedDirPath); + // if already known return the file - LocatableEntry* entry = _LookupEntry(EntryPath(directoryPath, name)); + LocatableEntry* entry = _LookupEntry(EntryPath(normalizedDirPath, name)); if (entry != NULL) { LocatableFile* file = dynamic_cast(entry); if (file == NULL) return NULL; - if (file->AcquireReference() == 0) { + if (file->AcquireReference() == 0) fEntries.Remove(file); - fDeadEntries.Insert(file); - } else + else return file; } // no such file yet -- create it - BString normalizedDirPath; - _NormalizePath(directoryPath, normalizedDirPath); LocatableDirectory* directory = _GetDirectory(normalizedDirPath); if (directory == NULL) return NULL; @@ -337,6 +323,7 @@ private: directory->AddEntry(file); fEntries.Insert(file); + return file; } @@ -488,7 +475,6 @@ private: private: FileManager* fManager; LocatableEntryTable fEntries; - DeadEntryList fDeadEntries; bool fIsLocal; }; diff --git a/src/kits/debugger/files/FileManager.h b/src/kits/debugger/files/FileManager.h index 7c47a98a4b..385d03a276 100644 --- a/src/kits/debugger/files/FileManager.h +++ b/src/kits/debugger/files/FileManager.h @@ -65,7 +65,6 @@ private: struct SourceFileHashDefinition; typedef BOpenHashTable LocatableEntryTable; - typedef DoublyLinkedList DeadEntryList; typedef BOpenHashTable SourceFileTable; typedef std::map LocatedFileMap; @@ -83,7 +82,6 @@ private: Domain* fTargetDomain; Domain* fSourceDomain; SourceFileTable* fSourceFiles; - LocatedFileMap fSourceLocationMappings; }; diff --git a/src/kits/debugger/files/LocatableDirectory.cpp b/src/kits/debugger/files/LocatableDirectory.cpp index e149da73c6..bf1920dbce 100644 --- a/src/kits/debugger/files/LocatableDirectory.cpp +++ b/src/kits/debugger/files/LocatableDirectory.cpp @@ -24,6 +24,9 @@ LocatableDirectory::~LocatableDirectory() const char* LocatableDirectory::Name() const { + if (fPath.Length() <= 1) + return fPath; + int32 lastSlash = fPath.FindLast('/'); // return -1, if not found return fPath.String() + (lastSlash + 1); diff --git a/src/kits/debugger/files/LocatableEntry.cpp b/src/kits/debugger/files/LocatableEntry.cpp index a4e2247961..ec89e14e8c 100644 --- a/src/kits/debugger/files/LocatableEntry.cpp +++ b/src/kits/debugger/files/LocatableEntry.cpp @@ -44,4 +44,5 @@ void LocatableEntry::LastReferenceReleased() { fOwner->LocatableEntryUnused(this); + delete this; } diff --git a/src/kits/debugger/model/StackFrame.cpp b/src/kits/debugger/model/StackFrame.cpp index 6cb760c7d2..86b8f1cf67 100644 --- a/src/kits/debugger/model/StackFrame.cpp +++ b/src/kits/debugger/model/StackFrame.cpp @@ -54,6 +54,12 @@ StackFrame::~StackFrame() fDebugInfo->ReleaseReference(); fCpuState->ReleaseReference(); + + if (fValues != NULL) + fValues->ReleaseReference(); + + if (fValueInfos != NULL) + fValueInfos->ReleaseReference(); } diff --git a/src/kits/debugger/settings/TeamSettings.cpp b/src/kits/debugger/settings/TeamSettings.cpp index 54a0480708..0b3a4323f6 100644 --- a/src/kits/debugger/settings/TeamSettings.cpp +++ b/src/kits/debugger/settings/TeamSettings.cpp @@ -44,6 +44,8 @@ TeamSettings::TeamSettings(const TeamSettings& other) TeamSettings::~TeamSettings() { _Unset(); + delete fFileManagerSettings; + delete fSignalSettings; } diff --git a/src/kits/debugger/value/ValueNodeContainer.cpp b/src/kits/debugger/value/ValueNodeContainer.cpp index 774aacf706..1fbb4e198e 100644 --- a/src/kits/debugger/value/ValueNodeContainer.cpp +++ b/src/kits/debugger/value/ValueNodeContainer.cpp @@ -69,6 +69,7 @@ ValueNodeContainer::RemoveChild(ValueNodeChild* child) if (child->Container() != this || !fChildren.RemoveItem(child)) return; + child->SetNode(NULL); child->SetContainer(NULL); child->ReleaseReference(); } @@ -78,6 +79,7 @@ void ValueNodeContainer::RemoveAllChildren() { for (int32 i = 0; ValueNodeChild* child = ChildAt(i); i++) { + child->SetNode(NULL); child->SetContainer(NULL); child->ReleaseReference(); } diff --git a/src/kits/debugger/value/value_nodes/BListValueNode.cpp b/src/kits/debugger/value/value_nodes/BListValueNode.cpp index 4a5a52dedf..9727e50c13 100644 --- a/src/kits/debugger/value/value_nodes/BListValueNode.cpp +++ b/src/kits/debugger/value/value_nodes/BListValueNode.cpp @@ -213,8 +213,6 @@ BListValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, // load the value data status_t error = B_OK; - _location = location; - _value = NULL; ValueLocation* memberLocation = NULL; CompoundType* baseType = dynamic_cast(fType); @@ -290,6 +288,10 @@ BListValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, memberLocation = NULL; } + location->AcquireReference(); + _location = location; + _value = NULL; + return B_OK; } diff --git a/src/kits/debugger/value/value_nodes/BMessageValueNode.cpp b/src/kits/debugger/value/value_nodes/BMessageValueNode.cpp index e65cc91087..0129c26b9d 100644 --- a/src/kits/debugger/value/value_nodes/BMessageValueNode.cpp +++ b/src/kits/debugger/value/value_nodes/BMessageValueNode.cpp @@ -164,9 +164,6 @@ BMessageValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, // load the value data status_t error = B_OK; - _location = location; - _value = NULL; - ValueLocation* memberLocation = NULL; BVariant headerAddress; @@ -318,6 +315,10 @@ BMessageValueNode::ResolvedLocationAndValue(ValueLoader* valueLoader, if (error != B_OK) return error; + location->AcquireReference(); + _location = location; + _value = NULL; + return B_OK; }