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/newview/llappviewer.cpp | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index b232a8c3bb..a76ac58724 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -1681,24 +1681,9 @@ bool LLAppViewer::cleanup() gDirUtilp->deleteFilesInDir(logdir, "*-*-*-*-*.dmp"); } - { - // Kill off LLLeap objects. We can find them all because LLLeap is derived - // from LLInstanceTracker. But collect instances first: LLInstanceTracker - // specifically forbids adding/deleting instances while iterating. - std::vector leaps; - leaps.reserve(LLLeap::instanceCount()); - for (LLLeap::instance_iter li(LLLeap::beginInstances()), lend(LLLeap::endInstances()); - li != lend; ++li) - { - leaps.push_back(&*li); - } - // Okay, now trash them all. We don't have to NULL or erase the entry - // in 'leaps' because the whole vector is going away momentarily. - BOOST_FOREACH(LLLeap* leap, leaps) - { - delete leap; - } - } // destroy 'leaps' + // Kill off LLLeap objects. We can find them all because LLLeap is derived + // from LLInstanceTracker. + LLLeap::instance_snapshot().deleteAll(); //flag all elements as needing to be destroyed immediately // to ensure shutdown order @@ -2858,12 +2843,11 @@ bool LLAppViewer::initConfiguration() // Let anyone else who cares know that we've populated our settings // variables. - for (LLControlGroup::key_iter ki(LLControlGroup::beginKeys()), kend(LLControlGroup::endKeys()); - ki != kend; ++ki) + for (const auto& key : LLControlGroup::key_snapshot()) { // For each named instance of LLControlGroup, send an event saying // we've initialized an LLControlGroup instance by that name. - LLEventPumps::instance().obtain("LLControlGroup").post(LLSDMap("init", *ki)); + LLEventPumps::instance().obtain("LLControlGroup").post(LLSDMap("init", key)); } return true; // Config was successful. -- cgit v1.2.3 From 47ec6ab3be5df5ee3f80a642d9c2ef7f4dac0d8a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 30 May 2019 08:23:32 -0400 Subject: SL-11216: Remove LLSingletonBase::cleanupAll(). Remove call from LLAppViewer::cleanup(). Instead, make each LLSingleton::deleteSingleton() call cleanupSingleton() just before destroying the instance. Since deleteSingleton() is not a destructor, it's fine to call cleanupSingleton() from there; and since deleteAll() calls deleteSingleton() on every remaining instance, the former cleanupAll() functionality has been subsumed into deleteAll(). Since cleanupSingleton() is now called at exactly one point in the instance's lifetime, we no longer need a bool indicating whether it has been called. The previous protocol of calling cleanupAll() before deleteAll() implemented a two-phase cleanup strategy for the application. That is no longer needed. Moreover, the cleanupAll() / deleteAll() sequence created a time window during which individual LLSingleton instances weren't usable (to the extent that their cleanupSingleton() methods released essential resources) but still existed -- so a getInstance() call would return the crippled instance rather than recreating it. Remove cleanupAll() calls from tests; adjust to new order of expected side effects: instead of A::cleanupSingleton(), B::cleanupSingleton(), ~A(), ~B(), now we get A::cleanupSingleton(), ~A(), B::cleanupSingleton(), ~B(). --- indra/newview/llappviewer.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index a76ac58724..ed80e7c0ce 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -2093,25 +2093,19 @@ bool LLAppViewer::cleanup() removeMarkerFiles(); - // It's not at first obvious where, in this long sequence, generic cleanup - // calls OUGHT to go. So let's say this: as we migrate cleanup from + // It's not at first obvious where, in this long sequence, a generic cleanup + // call OUGHT to go. So let's say this: as we migrate cleanup from // explicit hand-placed calls into the generic mechanism, eventually - // all cleanup will get subsumed into the generic calls. So the calls you + // all cleanup will get subsumed into the generic call. So the calls you // still see above are calls that MUST happen before the generic cleanup // kicks in. - // This calls every remaining LLSingleton's cleanupSingleton() method. - // This method should perform any cleanup that might take significant - // realtime, or might throw an exception. - LLSingletonBase::cleanupAll(); - // The logging subsystem depends on an LLSingleton. Any logging after // LLSingletonBase::deleteAll() won't be recorded. LL_INFOS() << "Goodbye!" << LL_ENDL; - // This calls every remaining LLSingleton's deleteSingleton() method. - // No class destructor should perform any cleanup that might take - // significant realtime, or throw an exception. + // This calls every remaining LLSingleton's cleanupSingleton() and + // deleteSingleton() methods. LLSingletonBase::deleteAll(); removeDumpDir(); -- cgit v1.2.3 From 5a260e0cc3beec45da1d29578855524977206022 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 30 May 2019 10:39:37 -0400 Subject: SL-11216: Convert LLVersionInfo to an LLSingleton. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This changeset is meant to exemplify how to convert a "namespace" class whose methods are static -- and whose data are module-static -- to an LLSingleton. LLVersionInfo has no initClass() or cleanupClass() methods, but the general idea is the same. * Derive the class from LLSingleton: class LLSomeSingleton: public LLSingleton { ... }; * Add LLSINGLETON(LLSomeSingleton); in the private section of the class. This usage implies a separate LLSomeSingleton::LLSomeSingleton() definition, as described in indra/llcommon/llsingleton.h. * Move module-scope data in the .cpp file to non-static class members. Change any sVariableName to mVariableName to avoid being outright misleading. * Make static class methods non-static. Remove '//static' comments from method definitions as needed. * For LLVersionInfo specifically, the 'const std::string&' return type was replaced with 'std::string'. Returning a reference to a static or a member, const or otherwise, is an anti-pattern: the interface constrains the implementation, prohibiting possibly later returning a temporary (an expression). * For LLVersionInfo specifically, 'const S32' return type was replaced with simple 'S32'. 'const' is just noise in that usage. * Simple member initialization (e.g. the original initializer expressions for static variables) can be done with member{ value } initializers (no examples here though). * Delete initClass() method. * LLSingleton's forté is of course lazy initialization. It might work to simply delete any calls to initClass(). But if there are side effects that must happen at that moment, replace LLSomeSingleton::initClass() with (void)LLSomeSingleton::instance(); * Most initClass() initialization can be done in the constructor, as would normally be the case. * Initialization that might cause a circular LLSingleton reference should be moved to initSingleton(). Override 'void initSingleton();' should be private. * For LLVersionInfo specifically, certain initialization that used to be lazily performed was made unconditional, due to its low cost. * For LLVersionInfo specifically, certain initialization involved calling methods that have become non-static. This was moved to initSingleton() because, in a constructor body, 'this' does not yet point to the enclosing class. * Delete cleanupClass() method. * There is already a generic LLSingletonBase::deleteAll() call in LLAppViewer::cleanup(). It might work to let this new LLSingleton be cleaned up with all the rest. But if there are side effects that must happen at that moment, replace LLSomeSingleton::cleanupClass() with LLSomeSingleton::deleteSingleton(). That said, much of the benefit of converting to LLSingleton is deleteAll()'s guarantee that cross-LLSingleton dependencies will be properly honored: we're trying to migrate the code base away from the present fragile manual cleanup sequence. * Most cleanupClass() cleanup can be done in the destructor, as would normally be the case. * Cleanup that might throw an exception should be moved to cleanupSingleton(). Override 'void cleanupSingleton();' should be private. * Within LLSomeSingleton methods, remove any existing LLSomeSingleton::methodName() qualification: simple methodName() is better. * In the rest of the code base, convert most LLSomeSingleton::methodName() references to LLSomeSingleton::instance().methodName(). (Prefer instance() to getInstance() because a reference does not admit the possibility of NULL.) * Of course, LLSomeSingleton::ENUM_VALUE can remain unchanged. In general, for many successive references to an LLSingleton instance, it can be useful to capture the instance() as in: auto& versionInfo{LLVersionInfo::instance()}; // ... versionInfo.getVersion() ... We did not do that here only to simplify the code review. The STRINGIZE(expression) macro encapsulates: std::ostringstream out; out << expression; return out.str(); We used that in a couple places. For LLVersionInfo specifically, lllogininstance_test.cpp used to dummy out a couple specific static methods. It's harder to dummy out LLSingleton::instance() references, so we add the real class to that test. --- indra/newview/llappviewer.cpp | 50 +++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 25 deletions(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index ed80e7c0ce..795d14740a 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -1120,7 +1120,7 @@ bool LLAppViewer::init() // Save the current version to the prefs file gSavedSettings.setString("LastRunVersion", - LLVersionInfo::getChannelAndVersion()); + LLVersionInfo::instance().getChannelAndVersion()); gSimLastTime = gRenderStartTime.getElapsedTimeF32(); gSimFrames = (F32)gFrameCount; @@ -1156,7 +1156,7 @@ bool LLAppViewer::init() // UpdaterServiceSettings updater.args.add(stringize(gSavedSettings.getU32("UpdaterServiceSetting"))); // channel - updater.args.add(LLVersionInfo::getChannel()); + updater.args.add(LLVersionInfo::instance().getChannel()); // testok updater.args.add(stringize(gSavedSettings.getBOOL("UpdaterWillingToTest"))); // ForceAddressSize @@ -2616,7 +2616,7 @@ bool LLAppViewer::initConfiguration() std::string CmdLineChannel(gSavedSettings.getString("CmdLineChannel")); if(! CmdLineChannel.empty()) { - LLVersionInfo::resetChannel(CmdLineChannel); + LLVersionInfo::instance().resetChannel(CmdLineChannel); } // If we have specified crash on startup, set the global so we'll trigger the crash at the right time @@ -3062,15 +3062,15 @@ LLSD LLAppViewer::getViewerInfo() const // LLFloaterAbout. LLSD info; LLSD version; - version.append(LLVersionInfo::getMajor()); - version.append(LLVersionInfo::getMinor()); - version.append(LLVersionInfo::getPatch()); - version.append(LLVersionInfo::getBuild()); + version.append(LLVersionInfo::instance().getMajor()); + version.append(LLVersionInfo::instance().getMinor()); + version.append(LLVersionInfo::instance().getPatch()); + version.append(LLVersionInfo::instance().getBuild()); info["VIEWER_VERSION"] = version; - info["VIEWER_VERSION_STR"] = LLVersionInfo::getVersion(); - info["CHANNEL"] = LLVersionInfo::getChannel(); + info["VIEWER_VERSION_STR"] = LLVersionInfo::instance().getVersion(); + info["CHANNEL"] = LLVersionInfo::instance().getChannel(); info["ADDRESS_SIZE"] = ADDRESS_SIZE; - std::string build_config = LLVersionInfo::getBuildConfig(); + std::string build_config = LLVersionInfo::instance().getBuildConfig(); if (build_config != "Release") { info["BUILD_CONFIG"] = build_config; @@ -3081,7 +3081,7 @@ LLSD LLAppViewer::getViewerInfo() const std::string url = LLTrans::getString("RELEASE_NOTES_BASE_URL"); if (! LLStringUtil::endsWith(url, "/")) url += "/"; - url += LLURI::escape(LLVersionInfo::getVersion()) + ".html"; + url += LLURI::escape(LLVersionInfo::instance().getVersion()) + ".html"; info["VIEWER_RELEASE_NOTES_URL"] = url; @@ -3372,12 +3372,12 @@ void LLAppViewer::writeSystemInfo() gDebugInfo["SLLog"] = gDirUtilp->getExpandedFilename(LL_PATH_LOGS,"SecondLife.old"); //LLError::logFileName(); #endif - gDebugInfo["ClientInfo"]["Name"] = LLVersionInfo::getChannel(); - gDebugInfo["ClientInfo"]["MajorVersion"] = LLVersionInfo::getMajor(); - gDebugInfo["ClientInfo"]["MinorVersion"] = LLVersionInfo::getMinor(); - gDebugInfo["ClientInfo"]["PatchVersion"] = LLVersionInfo::getPatch(); - gDebugInfo["ClientInfo"]["BuildVersion"] = LLVersionInfo::getBuild(); - gDebugInfo["ClientInfo"]["AddressSize"] = LLVersionInfo::getAddressSize(); + gDebugInfo["ClientInfo"]["Name"] = LLVersionInfo::instance().getChannel(); + gDebugInfo["ClientInfo"]["MajorVersion"] = LLVersionInfo::instance().getMajor(); + gDebugInfo["ClientInfo"]["MinorVersion"] = LLVersionInfo::instance().getMinor(); + gDebugInfo["ClientInfo"]["PatchVersion"] = LLVersionInfo::instance().getPatch(); + gDebugInfo["ClientInfo"]["BuildVersion"] = LLVersionInfo::instance().getBuild(); + gDebugInfo["ClientInfo"]["AddressSize"] = LLVersionInfo::instance().getAddressSize(); gDebugInfo["CAFilename"] = gDirUtilp->getCAFile(); @@ -3420,7 +3420,7 @@ void LLAppViewer::writeSystemInfo() // Dump some debugging info LL_INFOS("SystemInfo") << "Application: " << LLTrans::getString("APP_NAME") << LL_ENDL; - LL_INFOS("SystemInfo") << "Version: " << LLVersionInfo::getChannelAndVersion() << LL_ENDL; + LL_INFOS("SystemInfo") << "Version: " << LLVersionInfo::instance().getChannelAndVersion() << LL_ENDL; // Dump the local time and time zone time_t now; @@ -3642,7 +3642,7 @@ void LLAppViewer::handleViewerCrash() // static void LLAppViewer::recordMarkerVersion(LLAPRFile& marker_file) { - std::string marker_version(LLVersionInfo::getChannelAndVersion()); + std::string marker_version(LLVersionInfo::instance().getChannelAndVersion()); if ( marker_version.length() > MAX_MARKER_LENGTH ) { LL_WARNS_ONCE("MarkerFile") << "Version length ("<< marker_version.length()<< ")" @@ -3659,7 +3659,7 @@ bool LLAppViewer::markerIsSameVersion(const std::string& marker_name) const { bool sameVersion = false; - std::string my_version(LLVersionInfo::getChannelAndVersion()); + std::string my_version(LLVersionInfo::instance().getChannelAndVersion()); char marker_version[MAX_MARKER_LENGTH]; S32 marker_version_length; @@ -5517,12 +5517,12 @@ void LLAppViewer::handleLoginComplete() initMainloopTimeout("Mainloop Init"); // Store some data to DebugInfo in case of a freeze. - gDebugInfo["ClientInfo"]["Name"] = LLVersionInfo::getChannel(); + gDebugInfo["ClientInfo"]["Name"] = LLVersionInfo::instance().getChannel(); - gDebugInfo["ClientInfo"]["MajorVersion"] = LLVersionInfo::getMajor(); - gDebugInfo["ClientInfo"]["MinorVersion"] = LLVersionInfo::getMinor(); - gDebugInfo["ClientInfo"]["PatchVersion"] = LLVersionInfo::getPatch(); - gDebugInfo["ClientInfo"]["BuildVersion"] = LLVersionInfo::getBuild(); + gDebugInfo["ClientInfo"]["MajorVersion"] = LLVersionInfo::instance().getMajor(); + gDebugInfo["ClientInfo"]["MinorVersion"] = LLVersionInfo::instance().getMinor(); + gDebugInfo["ClientInfo"]["PatchVersion"] = LLVersionInfo::instance().getPatch(); + gDebugInfo["ClientInfo"]["BuildVersion"] = LLVersionInfo::instance().getBuild(); LLParcel* parcel = LLViewerParcelMgr::getInstance()->getAgentParcel(); if ( parcel && parcel->getMusicURL()[0]) -- cgit v1.2.3 From 15418f991a77416e765e985c39267bb6d4c51a6f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 31 May 2019 16:44:58 -0400 Subject: SL-11216: getViewerInfo() calls LLVersionInfo::getReleaseNotes(). Make LLAppViewer retrieve release notes from LLVersionInfo, rather than synthesizing the release-notes URL itself based on the viewer version string. --- indra/newview/llappviewer.cpp | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index 795d14740a..ef4500c01f 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -3061,16 +3061,12 @@ LLSD LLAppViewer::getViewerInfo() const // is available to a getInfo() caller as to the user opening // LLFloaterAbout. LLSD info; - LLSD version; - version.append(LLVersionInfo::instance().getMajor()); - version.append(LLVersionInfo::instance().getMinor()); - version.append(LLVersionInfo::instance().getPatch()); - version.append(LLVersionInfo::instance().getBuild()); - info["VIEWER_VERSION"] = version; - info["VIEWER_VERSION_STR"] = LLVersionInfo::instance().getVersion(); - info["CHANNEL"] = LLVersionInfo::instance().getChannel(); + auto& versionInfo{ LLVersionInfo::instance() }; + info["VIEWER_VERSION"] = LLSDArray(versionInfo.getMajor())(versionInfo.getMinor())(versionInfo.getPatch())(versionInfo.getBuild()); + info["VIEWER_VERSION_STR"] = versionInfo.getVersion(); + info["CHANNEL"] = versionInfo.getChannel(); info["ADDRESS_SIZE"] = ADDRESS_SIZE; - std::string build_config = LLVersionInfo::instance().getBuildConfig(); + std::string build_config = versionInfo.getBuildConfig(); if (build_config != "Release") { info["BUILD_CONFIG"] = build_config; @@ -3078,12 +3074,8 @@ LLSD LLAppViewer::getViewerInfo() const // return a URL to the release notes for this viewer, such as: // https://releasenotes.secondlife.com/viewer/2.1.0.123456.html - std::string url = LLTrans::getString("RELEASE_NOTES_BASE_URL"); - if (! LLStringUtil::endsWith(url, "/")) - url += "/"; - url += LLURI::escape(LLVersionInfo::instance().getVersion()) + ".html"; - - info["VIEWER_RELEASE_NOTES_URL"] = url; + std::string url = versionInfo.getReleaseNotes(); + info["VIEWER_RELEASE_NOTES_URL"] = url.empty()? LLTrans::getString("RetrievingData") : url; // Position LLViewerRegion* region = gAgent.getRegion(); -- cgit v1.2.3 From 8961be780d5c5ffa4f9cfc8060756a1bac5fc69f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 1 Jun 2019 09:30:36 -0400 Subject: SL-11216: Try to pacify VS 2013. --- indra/newview/llappviewer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index ef4500c01f..af70751b37 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -3061,7 +3061,7 @@ LLSD LLAppViewer::getViewerInfo() const // is available to a getInfo() caller as to the user opening // LLFloaterAbout. LLSD info; - auto& versionInfo{ LLVersionInfo::instance() }; + auto& versionInfo(LLVersionInfo::instance()); info["VIEWER_VERSION"] = LLSDArray(versionInfo.getMajor())(versionInfo.getMinor())(versionInfo.getPatch())(versionInfo.getBuild()); info["VIEWER_VERSION_STR"] = versionInfo.getVersion(); info["CHANNEL"] = versionInfo.getChannel(); -- 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/newview/llappviewer.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index af70751b37..db2db43ee1 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -1422,6 +1422,8 @@ bool LLAppViewer::doFrame() // canonical per-frame event mainloop.post(newFrame); + // give listeners a chance to run + llcoro::suspend(); if (!LLApp::isExiting()) { -- cgit v1.2.3 From 253e360062a74cbaf87c7f9dc30ef26bca1697eb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2019 16:36:16 -0400 Subject: DRTVWR-476: Mention LLApp::stepFrame() in LLAppViewer::idle() which performs "by hand" the same sequence of calls found in stepFrame(). Why not simply call stepFrame()? Hysterical reasons? --- indra/newview/llappviewer.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index db2db43ee1..1bba17dc29 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -4610,6 +4610,9 @@ void LLAppViewer::idle() LLFrameTimer::updateFrameTime(); LLFrameTimer::updateFrameCount(); LLEventTimer::updateClass(); + // LLApp::stepFrame() performs the above three calls plus mRunner.run(). + // Not sure why we don't call stepFrame() here, except that LLRunner seems + // completely redundant with LLEventTimer. LLNotificationsUI::LLToast::updateClass(); LLSmoothInterpolation::updateInterpolants(); LLMortician::updateClass(); -- cgit v1.2.3 From 9d5d257ceeb8297bcd8ac17164b6584717b5c024 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 14 May 2020 16:58:33 -0400 Subject: DRTVWR-476, SL-12204: Fix crash in Marketplace Listings. The observed crash was due to sharing a stateful global resource (the global LLMessageSystem instance) between different tasks. Specifically, a coroutine sets its mMessageReader one way, expecting that value to persist until it's done with message parsing, but another coroutine sneaks in at a suspension point and sets it differently. Introduce LockMessageReader and LockMessageChecker classes, which must be instantiated by a consumer of the resource. The constructor of each locks a coroutine-aware mutex, so that for the lifetime of the lock object no other coroutine can instantiate another. Refactor the code so that LLMessageSystem::mMessageReader can only be modified by LockMessageReader, not by direct assignment. mMessageReader is now an instance of LLMessageReaderPointer, which supports dereferencing and comparison but not assignment. Only LockMessageReader can change its value. LockMessageReader addresses the use case in which the specific mMessageReader value need only persist for the duration of a single method call. Add an instance in LLMessageHandlerBridge::post(). LockMessageChecker is a subclass of LockMessageReader: both lock the same mutex. LockMessageChecker addresses the use case in which the specific mMessageReader value must persist across multiple method calls. Modify the methods in question to require a LockMessageChecker instance. Provide LockMessageChecker forwarding methods to facilitate calling the underlying LLMessageSystem methods via the LockMessageChecker instance. Add LockMessageChecker instances to LLAppViewer::idleNetwork(), a couple cases in idle_startup() and LLMessageSystem::establishBidirectionalTrust(). --- indra/newview/llappviewer.cpp | 49 +++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 23 deletions(-) (limited to 'indra/newview/llappviewer.cpp') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index 65bfcec8c4..70d3c10524 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -5250,37 +5250,40 @@ void LLAppViewer::idleNetwork() const S64 frame_count = gFrameCount; // U32->S64 F32 total_time = 0.0f; - while (gMessageSystem->checkAllMessages(frame_count, gServicePump)) { - if (gDoDisconnect) + LockMessageChecker lmc(gMessageSystem); + while (lmc.checkAllMessages(frame_count, gServicePump)) { - // We're disconnecting, don't process any more messages from the server - // We're usually disconnecting due to either network corruption or a - // server going down, so this is OK. - break; - } + if (gDoDisconnect) + { + // We're disconnecting, don't process any more messages from the server + // We're usually disconnecting due to either network corruption or a + // server going down, so this is OK. + break; + } - total_decoded++; - gPacketsIn++; + total_decoded++; + gPacketsIn++; - if (total_decoded > MESSAGE_MAX_PER_FRAME) - { - break; - } + if (total_decoded > MESSAGE_MAX_PER_FRAME) + { + break; + } #ifdef TIME_THROTTLE_MESSAGES - // Prevent slow packets from completely destroying the frame rate. - // This usually happens due to clumps of avatars taking huge amount - // of network processing time (which needs to be fixed, but this is - // a good limit anyway). - total_time = check_message_timer.getElapsedTimeF32(); - if (total_time >= CheckMessagesMaxTime) - break; + // Prevent slow packets from completely destroying the frame rate. + // This usually happens due to clumps of avatars taking huge amount + // of network processing time (which needs to be fixed, but this is + // a good limit anyway). + total_time = check_message_timer.getElapsedTimeF32(); + if (total_time >= CheckMessagesMaxTime) + break; #endif - } + } - // Handle per-frame message system processing. - gMessageSystem->processAcks(gSavedSettings.getF32("AckCollectTime")); + // Handle per-frame message system processing. + lmc.processAcks(gSavedSettings.getF32("AckCollectTime")); + } #ifdef TIME_THROTTLE_MESSAGES if (total_time >= CheckMessagesMaxTime) -- cgit v1.2.3