From 9d5b897600a8f9405ec37a141b9417f34a11c557 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 2 Dec 2019 14:39:24 -0500 Subject: DRTVWR-494: Defend LLInstanceTracker against multi-thread usage. The previous implementation went to some effort to crash if anyone attempted to create or destroy an LLInstanceTracker subclass instance during traversal. That restriction is manageable within a single thread, but becomes unworkable if it's possible that a given subclass might be used on more than one thread. Remove LLInstanceTracker::instance_iter, beginInstances(), endInstances(), also key_iter, beginKeys() and endKeys(). Instead, introduce key_snapshot() and instance_snapshot(), the only means of iterating over LLInstanceTracker instances. (These are intended to resemble functions, but in fact the current implementation simply presents the classes.) Iterating over a captured snapshot defends against container modifications during traversal. The term 'snapshot' reminds the coder that a new instance created during traversal will not be considered. To defend against instance deletion during traversal, a snapshot stores std::weak_ptrs which it lazily dereferences, skipping on the fly any that have expired. Dereferencing instance_snapshot::iterator gets you a reference rather than a pointer. Because some use cases want to delete all existing instances, add an instance_snapshot::deleteAll() method that extracts the pointer. Those cases used to require explicitly copying instance pointers into a separate container; instance_snapshot() now takes care of that. It remains the caller's responsibility to ensure that all instances of that LLInstanceTracker subclass were allocated on the heap. Replace unkeyed static LLInstanceTracker::getInstance(T*) -- which returned nullptr if that instance had been destroyed -- with new getWeak() method returning std::weak_ptr. Caller must detect expiration of that weak_ptr. Adjust tests accordingly. Use of std::weak_ptr to detect expired instances requires engaging std::shared_ptr in the constructor. We now store shared_ptrs in the static containers (std::map for keyed, std::set for unkeyed). Make LLInstanceTrackerBase a template parameterized on the type of the static data it manages. For that reason, hoist static data class declarations out of the class definitions to an LLInstanceTrackerStuff namespace. Remove the static atomic sIterationNestDepth and its methods incrementDepth(), decrementDepth() and getDepth(), since they were used only to forbid creation and destruction during traversal. Add a std::mutex to static data. Introduce an internal LockStatic class that locks the mutex while providing a pointer to static data, making that the only way to access the static data. The LLINSTANCETRACKER_DTOR_NOEXCEPT macro goes away because we no longer expect ~LLInstanceTracker() to throw an exception in test programs. That affects LLTrace::StatBase as well as LLInstanceTracker itself. Adapt consumers to the new LLInstanceTracker API. --- indra/llui/llconsole.cpp | 4 ++-- indra/llui/lllayoutstack.cpp | 6 +++--- indra/llui/llnotificationslistener.cpp | 8 +++----- 3 files changed, 8 insertions(+), 10 deletions(-) (limited to 'indra/llui') diff --git a/indra/llui/llconsole.cpp b/indra/llui/llconsole.cpp index 5f50e46233..7817d99aef 100644 --- a/indra/llui/llconsole.cpp +++ b/indra/llui/llconsole.cpp @@ -369,9 +369,9 @@ LLConsole::Paragraph::Paragraph (LLWString str, const LLColor4 &color, F32 add_t // static void LLConsole::updateClass() { - for (instance_iter it = beginInstances(); it != endInstances(); ++it) + for (auto& con : instance_snapshot()) { - it->update(); + con.update(); } } diff --git a/indra/llui/lllayoutstack.cpp b/indra/llui/lllayoutstack.cpp index 4a464b3507..4aae1e374b 100644 --- a/indra/llui/lllayoutstack.cpp +++ b/indra/llui/lllayoutstack.cpp @@ -636,10 +636,10 @@ void LLLayoutStack::createResizeBar(LLLayoutPanel* panelp) //static void LLLayoutStack::updateClass() { - for (instance_iter it = beginInstances(); it != endInstances(); ++it) + for (auto& layout : instance_snapshot()) { - it->updateLayout(); - it->mAnimatedThisFrame = false; + layout.updateLayout(); + layout.mAnimatedThisFrame = false; } } diff --git a/indra/llui/llnotificationslistener.cpp b/indra/llui/llnotificationslistener.cpp index be26416cbb..e73ba1fbe9 100644 --- a/indra/llui/llnotificationslistener.cpp +++ b/indra/llui/llnotificationslistener.cpp @@ -127,18 +127,16 @@ void LLNotificationsListener::listChannels(const LLSD& params) const { LLReqID reqID(params); LLSD response(reqID.makeResponse()); - for (LLNotificationChannel::instance_iter cmi(LLNotificationChannel::beginInstances()), - cmend(LLNotificationChannel::endInstances()); - cmi != cmend; ++cmi) + for (auto& cm : LLNotificationChannel::instance_snapshot()) { LLSD channelInfo, parents; - BOOST_FOREACH(const std::string& parent, cmi->getParents()) + for (const std::string& parent : cm.getParents()) { parents.append(parent); } channelInfo["parents"] = parents; channelInfo["parent"] = parents.size()? parents[0] : ""; - response[cmi->getName()] = channelInfo; + response[cm.getName()] = channelInfo; } LLEventPumps::instance().obtain(params["reply"]).post(response); } -- cgit v1.2.3 From 66981fab0b3c8dcc3a031d50710a2b24ec6b0603 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 10 May 2018 21:46:07 -0400 Subject: SL-793: Use Boost.Fiber instead of the "dcoroutine" library. Longtime fans will remember that the "dcoroutine" library is a Google Summer of Code project by Giovanni P. Deretta. He originally called it "Boost.Coroutine," and we originally added it to our 3p-boost autobuild package as such. But when the official Boost.Coroutine library came along (with a very different API), and we still needed the API of the GSoC project, we renamed the unofficial one "dcoroutine" to allow coexistence. The "dcoroutine" library had an internal low-level API more or less analogous to Boost.Context. We later introduced an implementation of that internal API based on Boost.Context, a step towards eliminating the GSoC code in favor of official, supported Boost code. However, recent versions of Boost.Context no longer support the API on which we built the shim for "dcoroutine." We started down the path of reimplementing that shim using the current Boost.Context API -- then realized that it's time to bite the bullet and replace the "dcoroutine" API with the Boost.Fiber API, which we've been itching to do for literally years now. Naturally, most of the heavy lifting is in llcoros.{h,cpp} and lleventcoro.{h,cpp} -- which is good: the LLCoros layer abstracts away most of the differences between "dcoroutine" and Boost.Fiber. The one feature Boost.Fiber does not provide is the ability to forcibly terminate some other fiber. Accordingly, disable LLCoros::kill() and LLCoprocedureManager::shutdown(). The only known shutdown() call was in LLCoprocedurePool's destructor. We also took the opportunity to remove postAndSuspend2() and its associated machinery: FutureListener2, LLErrorEvent, errorException(), errorLog(), LLCoroEventPumps. All that dual-LLEventPump stuff was introduced at a time when the Responder pattern was king, and we assumed we'd want to listen on one LLEventPump with the success handler and on another with the error handler. We have never actually used that in practice. Remove associated tests, of course. There is one other semantic difference that necessitates patching a number of tests: with "dcoroutine," fulfilling a future IMMEDIATELY resumes the waiting coroutine. With Boost.Fiber, fulfilling a future merely marks the fiber as ready to resume next time the scheduler gets around to it. To observe the test side effects, we've inserted a number of llcoro::suspend() calls -- also in the main loop. For a long time we retained a single unit test exercising the raw "dcoroutine" API. Remove that. Eliminate llcoro_get_id.{h,cpp}, which provided llcoro::get_id(), which was a hack to emulate fiber-local variables. Since Boost.Fiber has an actual API for that, remove the hack. In fact, use (new alias) LLCoros::local_ptr for LLSingleton's dependency tracking in place of llcoro::get_id(). In CMake land, replace BOOST_COROUTINE_LIBRARY with BOOST_FIBER_LIBRARY. We don't actually use the Boost.Coroutine for anything (though there exist plausible use cases). --- indra/llui/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llui') diff --git a/indra/llui/CMakeLists.txt b/indra/llui/CMakeLists.txt index e44f57fa9f..2d2fa6588f 100644 --- a/indra/llui/CMakeLists.txt +++ b/indra/llui/CMakeLists.txt @@ -299,7 +299,7 @@ if(LL_TESTS) set(test_libs llui llmessage llcorehttp llcommon ${HUNSPELL_LIBRARY} ${LLCOMMON_LIBRARIES} - ${BOOST_COROUTINE_LIBRARY} ${BOOST_CONTEXT_LIBRARY} ${BOOST_SYSTEM_LIBRARY} + ${BOOST_FIBER_LIBRARY} ${BOOST_CONTEXT_LIBRARY} ${BOOST_SYSTEM_LIBRARY} ${WINDOWS_LIBRARIES}) if(NOT LINUX) LL_ADD_INTEGRATION_TEST(llurlentry llurlentry.cpp "${test_libs}") -- cgit v1.2.3 From 32f1dfa531062071ccf090b9c3d391b274caf02b Mon Sep 17 00:00:00 2001 From: Anchor Date: Mon, 10 Jun 2019 15:56:44 -0700 Subject: [DRTVWR-476] - fix compiler errors 32 bit windows build --- indra/llui/llaccordionctrl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llui') diff --git a/indra/llui/llaccordionctrl.cpp b/indra/llui/llaccordionctrl.cpp index 623f570cef..edcbc3fbb7 100644 --- a/indra/llui/llaccordionctrl.cpp +++ b/indra/llui/llaccordionctrl.cpp @@ -338,7 +338,7 @@ void LLAccordionCtrl::addCollapsibleCtrl(LLView* view) addChild(accordion_tab); mAccordionTabs.push_back(accordion_tab); - accordion_tab->setDropDownStateChangedCallback( boost::bind(&LLAccordionCtrl::onCollapseCtrlCloseOpen, this, mAccordionTabs.size() - 1) ); + accordion_tab->setDropDownStateChangedCallback( boost::bind(&LLAccordionCtrl::onCollapseCtrlCloseOpen, this, (S16)(mAccordionTabs.size() - 1)) ); arrange(); } -- cgit v1.2.3 From afaad3cef749e926cb63b6aed0c14f25d3495d55 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 15 Oct 2019 23:06:04 -0400 Subject: DRTVWR-476: Remove special case for listen(boost::bind(weak_ptr)). LLEventDetail::visit_and_connect() promised special treatment for the specific case when an LLEventPump::listen() listener was composed of (possibly nested) boost::bind() objects storing boost::weak_ptr values -- specifically boost::bind() rather than std::bind or lambdas, specifically boost::weak_ptr rather than std::weak_ptr. Outside of self-tests, it does not appear that anyone actually uses that support. There is good reason not to: it's a silent side effect of a complicated compile-time inspection that could be silently derailed by use of std::bind() or a lambda or a std::weak_ptr. Can you be sure you've engaged that promise? How? A more robust guarantee can be achieved by storing an LLTempBoundConnection in the transient object itself. When the object is destroyed, the listener is disconnected. Normal C++ rules around object destruction guarantee it. This idiom is widely used. There are a couple good reasons to remove the visit_and_connect() machinery: * boost::bind() and boost::weak_ptr do not constitute the wave of the future. Preferring those constructs to lambdas and std::weak_ptr penalizes new code, whether by silently failing or by discouraging use of modern idioms. * The visit_and_connect() machinery was always complicated, and apparently never very robust. Most of its promised features have been commented out over the years. Making the code base simpler, clearer and more maintainable is always a useful effect. LLEventDetail::visit_and_connect() was also used by the four LLNotificationChannelBase::connectMumble() methods. Streamline those as well. Of course, remove related test code. --- indra/llui/llnotifications.h | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) (limited to 'indra/llui') diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h index 62cf41256b..cac687f53d 100644 --- a/indra/llui/llnotifications.h +++ b/indra/llui/llnotifications.h @@ -746,42 +746,24 @@ public: virtual ~LLNotificationChannelBase() {} // you can also connect to a Channel, so you can be notified of // changes to this channel - template - LLBoundListener connectChanged(const LISTENER& slot) + LLBoundListener connectChanged(const LLEventListener& slot) { - // Examine slot to see if it binds an LLEventTrackable subclass, or a - // boost::shared_ptr to something, or a boost::weak_ptr to something. // Call this->connectChangedImpl() to actually connect it. - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectChangedImpl, - this, - _1)); + return connectChangedImpl(slot); } - template - LLBoundListener connectAtFrontChanged(const LISTENER& slot) + LLBoundListener connectAtFrontChanged(const LLEventListener& slot) { - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectAtFrontChangedImpl, - this, - _1)); + return connectAtFrontChangedImpl(slot); } - template - LLBoundListener connectPassedFilter(const LISTENER& slot) + LLBoundListener connectPassedFilter(const LLEventListener& slot) { // see comments in connectChanged() - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectPassedFilterImpl, - this, - _1)); + return connectPassedFilterImpl(slot); } - template - LLBoundListener connectFailedFilter(const LISTENER& slot) + LLBoundListener connectFailedFilter(const LLEventListener& slot) { // see comments in connectChanged() - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectFailedFilterImpl, - this, - _1)); + return connectFailedFilterImpl(slot); } // use this when items change or to add a new one -- cgit v1.2.3