From a1eccae96f09199398ff1ad573ce60b62f989519 Mon Sep 17 00:00:00 2001 From: Markus Himmel Date: Sun, 8 Nov 2015 15:16:05 +0000 Subject: [PATCH] Make sure images containing BTranslators are not unloaded early MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a translator is uninstalled, BTranslatorPrivate::_RemoveTranslators is called. This method used to unload the image containing the translator after calling Release() on it resulting in several problems: - If the translator was still busy, e.g. translating something while being installed, it crashed since the image was unloaded even though its refcount was larger than 0. - Applications using code from one of the translators (e.g. its config view) would crash when the translator is uninstalled (this is bug #12005). This problem is now fixed. The roster keeps track of all translators whose image it manages (even if the translator was already removed from the roster). It also keeps a refcount to all images. When a translator's refcount drops to zero and it belonged to a roster at some point, it does not delete itself, but notifies the roster that it is ready to destruct, which then removes it from the roster if the translator is still in it, destroys the translator, decrements the refcount of the image and if the new refcount is zero, unloads the image. All of this is done in a message handler, since if the translator called TranslatorDeleted like before, the unloaded image would be referenced when the stack is walked up. Finally, the DataTranslations preflet is required to Acquire() the translator whose config view it is showing, because otherwise its refcount could be reduced to 0 and the image unloaded. BTranslatorRoster now enables users to acquire a translator by ID. By the time the translator has to be released, it might not be part of the roster anymore though. Since BTranslatorRoster tries not to give out raw pointers to the translators it manages, users who acquire a translator through a roster now are given a BTranslatorReleaseDelegate, which allows for releasing the BTranslator exactly once and then self-destructs. Signed-off-by: Axel Dörfler --- headers/os/app/AppDefs.h | 1 + headers/os/translation/TranslatorRoster.h | 13 +++ src/kits/translation/Translator.cpp | 20 ++++- src/kits/translation/TranslatorRoster.cpp | 79 +++++++++++++++---- .../translation/TranslatorRosterPrivate.h | 8 +- .../DataTranslationsWindow.cpp | 16 +++- .../datatranslations/DataTranslationsWindow.h | 2 + 7 files changed, 115 insertions(+), 24 deletions(-) diff --git a/headers/os/app/AppDefs.h b/headers/os/app/AppDefs.h index 77383ef39a..0a70d8b432 100644 --- a/headers/os/app/AppDefs.h +++ b/headers/os/app/AppDefs.h @@ -59,6 +59,7 @@ enum { B_VALUE_CHANGED = '_VCH', B_TRANSLATOR_ADDED = '_ART', B_TRANSLATOR_REMOVED = '_RRT', + B_DELETE_TRANSLATOR = '_DRT', B_VIEW_MOVED = '_VMV', B_VIEW_RESIZED = '_VRS', B_WINDOW_MOVED = '_WMV', diff --git a/headers/os/translation/TranslatorRoster.h b/headers/os/translation/TranslatorRoster.h index 4d95853ed3..b01d9c2701 100644 --- a/headers/os/translation/TranslatorRoster.h +++ b/headers/os/translation/TranslatorRoster.h @@ -23,6 +23,17 @@ class BView; struct entry_ref; +class BTranslatorReleaseDelegate { +public: + BTranslatorReleaseDelegate(BTranslator* translator); + + void Release(); + +private: + BTranslator* fUnderlying; +}; + + class BTranslatorRoster : public BArchivable { public: BTranslatorRoster(); @@ -86,6 +97,8 @@ public: translator_id translatorID, BMessage* ioExtension); + BTranslatorReleaseDelegate* AcquireTranslator(int32 translatorID); + status_t GetRefFor(translator_id translatorID, entry_ref* ref); bool IsTranslator(entry_ref* ref); diff --git a/src/kits/translation/Translator.cpp b/src/kits/translation/Translator.cpp index 5699901c42..6afb80cfe0 100644 --- a/src/kits/translation/Translator.cpp +++ b/src/kits/translation/Translator.cpp @@ -24,8 +24,6 @@ BTranslator::BTranslator() BTranslator::~BTranslator() { - if (fOwningRoster != NULL) - fOwningRoster->TranslatorDeleted(fID); } @@ -52,10 +50,24 @@ BTranslator *BTranslator::Acquire() BTranslator *BTranslator::Release() { int32 oldValue = atomic_add(&fRefCount, -1); - if (oldValue > 0) + if (oldValue > 1) return this; - delete this; + if (fOwningRoster == NULL) { + delete this; + return NULL; + } + + // If we have ever been part of a roster, notify the roster to delete us + // and unload our image in a thread-safe way + BMessage deleteRequest(B_DELETE_TRANSLATOR); + + deleteRequest.AddPointer("ptr", this); + deleteRequest.AddInt32("id", fID); + + BMessenger sender(fOwningRoster); + sender.SendMessage(&deleteRequest); + return NULL; } diff --git a/src/kits/translation/TranslatorRoster.cpp b/src/kits/translation/TranslatorRoster.cpp index a0b54af4af..4e3f7c2c6e 100644 --- a/src/kits/translation/TranslatorRoster.cpp +++ b/src/kits/translation/TranslatorRoster.cpp @@ -193,9 +193,6 @@ BTranslatorRoster::Private::~Private() while (iterator != fTranslators.end()) { BTranslator* translator = iterator->second.translator; - translator->fOwningRoster = NULL; - // we don't want to be notified about this anymore - images.insert(iterator->second.image); translator->Release(); @@ -318,6 +315,19 @@ BTranslatorRoster::Private::MessageReceived(BMessage* message) break; } + case B_DELETE_TRANSLATOR: + { + // A translator's refcount has been reduced to zero and it wants + // us to delete it. + int32 id; + void* self; + if (message->FindInt32("id", &id) == B_OK + && message->FindPointer("ptr", &self) == B_OK) { + _TranslatorDeleted(id, (BTranslator*)self); + } + break; + } + default: BHandler::MessageReceived(message); break; @@ -592,6 +602,7 @@ BTranslatorRoster::Private::CreateTranslators(const entry_ref& ref, if (AddTranslator(translator, image, &ref, nodeRef.node) == B_OK) { if (update) update->AddInt32("translator_id", translator->fID); + fImageOrigins.insert(std::make_pair(translator, image)); count++; created++; } else { @@ -600,8 +611,12 @@ BTranslatorRoster::Private::CreateTranslators(const entry_ref& ref, } } - if (created == 0) + if (created == 0) { unload_add_on(image); + } else { + // Initial refcount for the image that was just loaded + fKnownImages.insert(std::make_pair(image, created)); + } quarantine.Remove(); return B_OK; @@ -850,18 +865,27 @@ BTranslatorRoster::Private::GetRefFor(translator_id id, entry_ref& ref) void -BTranslatorRoster::Private::TranslatorDeleted(translator_id id) +BTranslatorRoster::Private::_TranslatorDeleted(translator_id id, BTranslator* self) { BAutolock locker(this); TranslatorMap::iterator iterator = fTranslators.find(id); - if (iterator == fTranslators.end()) - return; + if (iterator != fTranslators.end()) + fTranslators.erase(iterator); - fTranslators.erase(iterator); + image_id image = fImageOrigins[self]; + + delete self; + + int32 former = atomic_add(&fKnownImages[image], -1); + if (former == 1) + { + unload_add_on(image); + fImageOrigins.erase(self); + fKnownImages.erase(image); + } } - /*static*/ int BTranslatorRoster::Private::_CompareSupport(const void* _a, const void* _b) { @@ -1073,9 +1097,6 @@ BTranslatorRoster::Private::_RemoveTranslators(const node_ref* nodeRef, if ((ref != NULL && item.ref == *ref) || (nodeRef != NULL && item.ref.device == nodeRef->device && item.node == nodeRef->node)) { - item.translator->fOwningRoster = NULL; - // if the translator is busy, we don't want to be notified - // about the removal later on item.translator->Release(); image = item.image; update.AddInt32("translator_id", iterator->first); @@ -1086,11 +1107,6 @@ BTranslatorRoster::Private::_RemoveTranslators(const node_ref* nodeRef, iterator = next; } - // Unload image from the removed translator - - if (image >= B_OK) - unload_add_on(image); - _NotifyListeners(update); } @@ -1150,6 +1166,21 @@ BTranslatorRoster::Private::_NotifyListeners(BMessage& update) const // #pragma mark - +BTranslatorReleaseDelegate::BTranslatorReleaseDelegate(BTranslator* translator) + : + fUnderlying(translator) +{ +} + + +void +BTranslatorReleaseDelegate::Release() +{ + fUnderlying->Release(); + // ReleaseDelegate is only allowed to release a translator once. + delete this; +} + BTranslatorRoster::BTranslatorRoster() { @@ -1665,6 +1696,20 @@ BTranslatorRoster::MakeConfigurationView(translator_id id, } +BTranslatorReleaseDelegate* +BTranslatorRoster::AcquireTranslator(int32 id) +{ + BAutolock locker(fPrivate); + + BTranslator* translator = fPrivate->FindTranslator(id); + if (translator == NULL) + return NULL; + + translator->Acquire(); + return new BTranslatorReleaseDelegate(translator); +} + + /*! Gets the configuration setttings for the translator specified by \a id and puts them into \a ioExtension. diff --git a/src/kits/translation/TranslatorRosterPrivate.h b/src/kits/translation/TranslatorRosterPrivate.h index dac2c01ef1..ceb9d3d94c 100644 --- a/src/kits/translation/TranslatorRosterPrivate.h +++ b/src/kits/translation/TranslatorRosterPrivate.h @@ -33,6 +33,8 @@ typedef std::map TranslatorMap; typedef std::vector MessengerList; typedef std::vector NodeRefList; typedef std::set EntryRefSet; +typedef std::map ImageMap; +typedef std::map TranslatorImageMap; class BTranslatorRoster::Private : public BHandler, public BLocker { @@ -78,8 +80,6 @@ public: status_t StartWatching(BMessenger target); status_t StopWatching(BMessenger target); - void TranslatorDeleted(translator_id id); - private: static int _CompareSupport(const void* _a, const void* _b); @@ -107,11 +107,15 @@ private: const char* name); void _EntryAdded(const entry_ref& ref); void _NotifyListeners(BMessage& update) const; + void _TranslatorDeleted(translator_id id, + BTranslator *self); NodeRefList fDirectories; TranslatorMap fTranslators; MessengerList fMessengers; EntryRefSet fRescanEntries; + ImageMap fKnownImages; + TranslatorImageMap fImageOrigins; const char* fABISubDirectory; int32 fNextID; bool fLazyScanning; diff --git a/src/preferences/datatranslations/DataTranslationsWindow.cpp b/src/preferences/datatranslations/DataTranslationsWindow.cpp index 24b83e58c3..c1ce98299c 100644 --- a/src/preferences/datatranslations/DataTranslationsWindow.cpp +++ b/src/preferences/datatranslations/DataTranslationsWindow.cpp @@ -53,7 +53,8 @@ DataTranslationsWindow::DataTranslationsWindow() : BWindow(BRect(0, 0, 550, 350), B_TRANSLATE_SYSTEM_NAME("DataTranslations"), B_TITLED_WINDOW, B_ASYNCHRONOUS_CONTROLS | B_NOT_ZOOMABLE - | B_NOT_RESIZABLE | B_AUTO_UPDATE_SIZE_LIMITS) + | B_NOT_RESIZABLE | B_AUTO_UPDATE_SIZE_LIMITS), + fRelease(NULL) { MoveTo(DataTranslationsSettings::Instance()->WindowCorner()); @@ -151,6 +152,10 @@ DataTranslationsWindow::_ShowConfigView(int32 id) fRightBox->RemoveChild(fConfigView); delete fConfigView; fConfigView = NULL; + if (fRelease != NULL) { + fRelease->Release(); + fRelease = NULL; + } } BMessage emptyMsg; @@ -165,6 +170,10 @@ DataTranslationsWindow::_ShowConfigView(int32 id) // force config views to all have the same color fRightBox->AddChild(fConfigView); + // Make sure the translator's image doesn't get unloaded while we are still + // showing a config view whose code is in the image + fRelease = roster->AcquireTranslator(id); + return B_OK; } @@ -176,6 +185,11 @@ DataTranslationsWindow::_ShowInfoView() fRightBox->RemoveChild(fConfigView); delete fConfigView; fConfigView = NULL; + if (fRelease != NULL) { + fRelease->Release(); + fRelease = NULL; + } + } BTextView* view = new BTextView("info text"); diff --git a/src/preferences/datatranslations/DataTranslationsWindow.h b/src/preferences/datatranslations/DataTranslationsWindow.h index 68e420750b..1e1f634e39 100644 --- a/src/preferences/datatranslations/DataTranslationsWindow.h +++ b/src/preferences/datatranslations/DataTranslationsWindow.h @@ -20,6 +20,7 @@ #include "TranslatorListView.h" +class BTranslatorReleaseDelegate; class DataTranslationsWindow : public BWindow { public: @@ -39,6 +40,7 @@ private: void _SetupViews(); TranslatorListView* fTranslatorListView; + BTranslatorReleaseDelegate* fRelease; BBox* fRightBox; BView* fConfigView;