From 1ea42d88f77867228c69ed15c7a38a96b3a03911 Mon Sep 17 00:00:00 2001 From: Anchor Date: Tue, 30 Apr 2019 16:53:21 -0600 Subject: [DRTVWR-476] - compile error fix --- indra/viewer_components/login/lllogin.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'indra/viewer_components/login/lllogin.cpp') diff --git a/indra/viewer_components/login/lllogin.cpp b/indra/viewer_components/login/lllogin.cpp index 9193d32b49..1a06459318 100644 --- a/indra/viewer_components/login/lllogin.cpp +++ b/indra/viewer_components/login/lllogin.cpp @@ -23,6 +23,7 @@ * $/LicenseInfo$ */ +#include "llwin32headers.h" #include "linden_common.h" #include "llsd.h" #include "llsdutil.h" -- cgit v1.3 From 2a56ab44360aa49bd0df9281efc8a8a97a510013 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 22 Nov 2019 11:58:27 -0500 Subject: DRTVWR-476, SL-12197: Don't throw Stopping from main coroutine. The new LLCoros::Stop exception is intended to terminate long-lived coroutines -- not interrupt mainstream shutdown processing. Only throw it on an explicitly-launched coroutine. Make LLCoros::getName() (used by the above test) static. As with other LLCoros methods, it might be called after the LLCoros LLSingleton instance has been deleted. Requiring the caller to call instance() implies a possible need to also call wasDeleted(). Encapsulate that nuance into a static method instead. --- indra/llcommon/llcoros.cpp | 12 +++++++++++- indra/llcommon/llcoros.h | 4 ++-- indra/llcommon/lleventcoro.cpp | 2 +- indra/llmessage/llavatarnamecache.cpp | 4 ++-- indra/newview/llaccountingcostmanager.cpp | 4 ++-- indra/viewer_components/login/lllogin.cpp | 4 ++-- 6 files changed, 20 insertions(+), 10 deletions(-) (limited to 'indra/viewer_components/login/lllogin.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index febe74b559..5f940de52b 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -192,7 +192,8 @@ bool LLCoros::kill(const std::string& name) } |*==========================================================================*/ -std::string LLCoros::getName() const +//static +std::string LLCoros::getName() { return get_CoroData("getName()").mName; } @@ -320,12 +321,21 @@ void LLCoros::toplevel(std::string name, callable_t callable) } } +//static void LLCoros::checkStop() { if (wasDeleted()) { LLTHROW(Shutdown("LLCoros was deleted")); } + // do this AFTER the check above, because getName() depends on + // get_CoroData(), which depends on the local_ptr in our instance(). + if (getName().empty()) + { + // Our Stop exception and its subclasses are intended to stop loitering + // coroutines. Don't throw it from the main coroutine. + return; + } if (LLApp::isStopped()) { LLTHROW(Stopped("viewer is stopped")); diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index 7b3420cc8f..2e4cd8ccad 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -140,7 +140,7 @@ public: * (e.g. if the coroutine was launched by hand rather than using * LLCoros::launch()). */ - std::string getName() const; + static std::string getName(); /** * For delayed initialization. To be clear, this will only affect @@ -295,7 +295,7 @@ inline std::string logname() { static std::string main("main"); - std::string name(LLCoros::instance().getName()); + std::string name(LLCoros::getName()); return name.empty()? main : name; } diff --git a/indra/llcommon/lleventcoro.cpp b/indra/llcommon/lleventcoro.cpp index 967c4d74d8..11b6e5bb2f 100644 --- a/indra/llcommon/lleventcoro.cpp +++ b/indra/llcommon/lleventcoro.cpp @@ -62,7 +62,7 @@ namespace std::string listenerNameForCoro() { // If this coroutine was launched by LLCoros::launch(), find that name. - std::string name(LLCoros::instance().getName()); + std::string name(LLCoros::getName()); if (! name.empty()) { return name; diff --git a/indra/llmessage/llavatarnamecache.cpp b/indra/llmessage/llavatarnamecache.cpp index 6a287f0cc5..fbd65cc67b 100644 --- a/indra/llmessage/llavatarnamecache.cpp +++ b/indra/llmessage/llavatarnamecache.cpp @@ -134,7 +134,7 @@ LLAvatarNameCache::~LLAvatarNameCache() void LLAvatarNameCache::requestAvatarNameCache_(std::string url, std::vector agentIds) { - LL_DEBUGS("AvNameCache") << "Entering coroutine " << LLCoros::instance().getName() + LL_DEBUGS("AvNameCache") << "Entering coroutine " << LLCoros::getName() << " with url '" << url << "', requesting " << agentIds.size() << " Agent Ids" << LL_ENDL; // Check pointer that can be cleaned up by cleanupClass() @@ -188,7 +188,7 @@ void LLAvatarNameCache::requestAvatarNameCache_(std::string url, std::vector observerHandle) { - LL_DEBUGS("LLAccountingCostManager") << "Entering coroutine " << LLCoros::instance().getName() + LL_DEBUGS("LLAccountingCostManager") << "Entering coroutine " << LLCoros::getName() << " with url '" << url << LL_ENDL; LLCore::HttpRequest::policy_t httpPolicy(LLCore::HttpRequest::DEFAULT_POLICY_ID); @@ -158,7 +158,7 @@ void LLAccountingCostManager::accountingCostCoro(std::string url, } catch (...) { - LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::instance().getName() + LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName() << "('" << url << "')")); throw; } diff --git a/indra/viewer_components/login/lllogin.cpp b/indra/viewer_components/login/lllogin.cpp index 1a06459318..57a7c03525 100644 --- a/indra/viewer_components/login/lllogin.cpp +++ b/indra/viewer_components/login/lllogin.cpp @@ -148,7 +148,7 @@ void LLLogin::Impl::loginCoro(std::string uri, LLSD login_params) } try { - LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::instance().getName() + LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::getName() << " with uri '" << uri << "', parameters " << printable_params << LL_ENDL; LLEventPump& xmlrpcPump(LLEventPumps::instance().obtain("LLXMLRPCTransaction")); @@ -307,7 +307,7 @@ void LLLogin::Impl::loginCoro(std::string uri, LLSD login_params) sendProgressEvent("offline", "fail.login", error_response); } catch (...) { - CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::instance().getName() + CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName() << "('" << uri << "', " << printable_params << ")")); } } -- cgit v1.3 From 87da08b1f49a600b0e1993b31e46b1808aa8fecf Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Jul 2020 14:48:36 -0400 Subject: DRTVWR-476, SL-13555: Don't crash if user closes viewer during login. Ever since February 2010, the body of the login coroutine function has been enclosed in try/catch (...), with an llerrs message to try to crash more informatively than the runtime's unhandled-exception termination. Over the years this evolved to LL_ERRS and then to CRASH_ON_UNHANDLED_EXCEPTION. This persisted despite the August 2016 addition of generic catch clauses in the LLCoros::toplevel() function to serve the same purpose, and despite the subsequent introduction of the LLCoros::Stop family of exceptions to deliberately throw into waiting coroutines on viewer shutdown. That's exactly what was happening. When the user closed the viewer while waiting for the response from login.cgi, the waiting operation threw LLCoros::Stopping, which was caught by that CRASH_ON_UNHANDLED_EXCEPTION, which crashed the viewer with LL_ERRS rather than propagating up to the toplevel() and cleanly terminating the coroutine. Change CRASH_ON_UNHANDLED_EXCEPTION() to LOG_UNHANDLED_EXCEPTION() and re-throw so toplevel() can handle. --- indra/llcommon/llcoros.h | 5 + indra/viewer_components/login/lllogin.cpp | 283 +++++++++++++++--------------- 2 files changed, 148 insertions(+), 140 deletions(-) (limited to 'indra/viewer_components/login/lllogin.cpp') diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index d49a6e939c..38c2356c99 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -232,6 +232,11 @@ public: }; /// thrown by checkStop() + // It may sound ironic that Stop is derived from LLContinueError, but the + // point is that LLContinueError is the category of exception that should + // not immediately crash the viewer. Stop and its subclasses are to notify + // coroutines that the viewer intends to shut down. The expected response + // is to terminate the coroutine, rather than abort the viewer. struct Stop: public LLContinueError { Stop(const std::string& what): LLContinueError(what) {} diff --git a/indra/viewer_components/login/lllogin.cpp b/indra/viewer_components/login/lllogin.cpp index 57a7c03525..d485203fa1 100644 --- a/indra/viewer_components/login/lllogin.cpp +++ b/indra/viewer_components/login/lllogin.cpp @@ -148,167 +148,170 @@ void LLLogin::Impl::loginCoro(std::string uri, LLSD login_params) } try { - LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::getName() - << " with uri '" << uri << "', parameters " << printable_params << LL_ENDL; + LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::getName() + << " with uri '" << uri << "', parameters " << printable_params << LL_ENDL; - LLEventPump& xmlrpcPump(LLEventPumps::instance().obtain("LLXMLRPCTransaction")); - // EXT-4193: use a DIFFERENT reply pump than for the SRV request. We used - // to share them -- but the EXT-3934 fix made it possible for an abandoned - // SRV response to arrive just as we were expecting the XMLRPC response. - LLEventStream loginReplyPump("loginreply", true); + LLEventPump& xmlrpcPump(LLEventPumps::instance().obtain("LLXMLRPCTransaction")); + // EXT-4193: use a DIFFERENT reply pump than for the SRV request. We used + // to share them -- but the EXT-3934 fix made it possible for an abandoned + // SRV response to arrive just as we were expecting the XMLRPC response. + LLEventStream loginReplyPump("loginreply", true); - LLSD::Integer attempts = 0; + LLSD::Integer attempts = 0; - LLSD request(login_params); - request["reply"] = loginReplyPump.getName(); - request["uri"] = uri; - std::string status; + LLSD request(login_params); + request["reply"] = loginReplyPump.getName(); + request["uri"] = uri; + std::string status; - // Loop back to here if login attempt redirects to a different - // request["uri"] - for (;;) - { - ++attempts; - LLSD progress_data; - progress_data["attempt"] = attempts; - progress_data["request"] = request; - if (progress_data["request"].has("params") - && progress_data["request"]["params"].has("passwd")) - { - progress_data["request"]["params"]["passwd"] = "*******"; - } - sendProgressEvent("offline", "authenticating", progress_data); - - // We expect zero or more "Downloading" status events, followed by - // exactly one event with some other status. Use postAndSuspend() the - // first time, because -- at least in unit-test land -- it's - // possible for the reply to arrive before the post() call - // returns. Subsequent responses, of course, must be awaited - // without posting again. - for (mAuthResponse = validateResponse(loginReplyPump.getName(), - llcoro::postAndSuspend(request, xmlrpcPump, loginReplyPump, "reply")); - mAuthResponse["status"].asString() == "Downloading"; - mAuthResponse = validateResponse(loginReplyPump.getName(), - llcoro::suspendUntilEventOn(loginReplyPump))) + // Loop back to here if login attempt redirects to a different + // request["uri"] + for (;;) { - // Still Downloading -- send progress update. - sendProgressEvent("offline", "downloading"); - } + ++attempts; + LLSD progress_data; + progress_data["attempt"] = attempts; + progress_data["request"] = request; + if (progress_data["request"].has("params") + && progress_data["request"]["params"].has("passwd")) + { + progress_data["request"]["params"]["passwd"] = "*******"; + } + sendProgressEvent("offline", "authenticating", progress_data); + + // We expect zero or more "Downloading" status events, followed by + // exactly one event with some other status. Use postAndSuspend() the + // first time, because -- at least in unit-test land -- it's + // possible for the reply to arrive before the post() call + // returns. Subsequent responses, of course, must be awaited + // without posting again. + for (mAuthResponse = validateResponse(loginReplyPump.getName(), + llcoro::postAndSuspend(request, xmlrpcPump, loginReplyPump, "reply")); + mAuthResponse["status"].asString() == "Downloading"; + mAuthResponse = validateResponse(loginReplyPump.getName(), + llcoro::suspendUntilEventOn(loginReplyPump))) + { + // Still Downloading -- send progress update. + sendProgressEvent("offline", "downloading"); + } - LL_DEBUGS("LLLogin") << "Auth Response: " << mAuthResponse << LL_ENDL; - status = mAuthResponse["status"].asString(); + LL_DEBUGS("LLLogin") << "Auth Response: " << mAuthResponse << LL_ENDL; + status = mAuthResponse["status"].asString(); - // Okay, we've received our final status event for this - // request. Unless we got a redirect response, break the retry - // loop for the current rewrittenURIs entry. - if (!(status == "Complete" && - mAuthResponse["responses"]["login"].asString() == "indeterminate")) - { - break; - } + // Okay, we've received our final status event for this + // request. Unless we got a redirect response, break the retry + // loop for the current rewrittenURIs entry. + if (!(status == "Complete" && + mAuthResponse["responses"]["login"].asString() == "indeterminate")) + { + break; + } - sendProgressEvent("offline", "indeterminate", mAuthResponse["responses"]); + sendProgressEvent("offline", "indeterminate", mAuthResponse["responses"]); - // Here the login service at the current URI is redirecting us - // to some other URI ("indeterminate" -- why not "redirect"?). - // The response should contain another uri to try, with its - // own auth method. - request["uri"] = mAuthResponse["responses"]["next_url"].asString(); - request["method"] = mAuthResponse["responses"]["next_method"].asString(); - } // loop back to try the redirected URI + // Here the login service at the current URI is redirecting us + // to some other URI ("indeterminate" -- why not "redirect"?). + // The response should contain another uri to try, with its + // own auth method. + request["uri"] = mAuthResponse["responses"]["next_url"].asString(); + request["method"] = mAuthResponse["responses"]["next_method"].asString(); + } // loop back to try the redirected URI - // Here we're done with redirects. - if (status == "Complete") - { - // StatusComplete does not imply auth success. Check the - // actual outcome of the request. We've already handled the - // "indeterminate" case in the loop above. - if (mAuthResponse["responses"]["login"].asString() == "true") - { - sendProgressEvent("online", "connect", mAuthResponse["responses"]); - } - else + // Here we're done with redirects. + if (status == "Complete") { - // Synchronize here with the updater. We synchronize here rather - // than in the fail.login handler, which actually examines the - // response from login.cgi, because here we are definitely in a - // coroutine and can definitely use suspendUntilBlah(). Whoever's - // listening for fail.login might not be. - - // If the reason for login failure is that we must install a - // required update, we definitely want to pass control to the - // updater to manage that for us. We'll handle any other login - // failure ourselves, as usual. We figure that no matter where you - // are in the world, or what kind of network you're on, we can - // reasonably expect the Viewer Version Manager to respond more or - // less as quickly as login.cgi. This synchronization is only - // intended to smooth out minor races between the two services. - // But what if the updater crashes? Use a timeout so that - // eventually we'll tire of waiting for it and carry on as usual. - // Given the above, it can be a fairly short timeout, at least - // from a human point of view. - - // Since sSyncPoint is an LLEventMailDrop, we DEFINITELY want to - // consume the posted event. - LLCoros::OverrideConsuming oc(true); - // Timeout should produce the isUndefined() object passed here. - LL_DEBUGS("LLLogin") << "Login failure, waiting for sync from updater" << LL_ENDL; - LLSD updater = llcoro::suspendUntilEventOnWithTimeout(sSyncPoint, 10, LLSD()); - if (updater.isUndefined()) + // StatusComplete does not imply auth success. Check the + // actual outcome of the request. We've already handled the + // "indeterminate" case in the loop above. + if (mAuthResponse["responses"]["login"].asString() == "true") { - LL_WARNS("LLLogin") << "Failed to hear from updater, proceeding with fail.login" - << LL_ENDL; + sendProgressEvent("online", "connect", mAuthResponse["responses"]); } else { - LL_DEBUGS("LLLogin") << "Got responses from updater and login.cgi" << LL_ENDL; + // Synchronize here with the updater. We synchronize here rather + // than in the fail.login handler, which actually examines the + // response from login.cgi, because here we are definitely in a + // coroutine and can definitely use suspendUntilBlah(). Whoever's + // listening for fail.login might not be. + + // If the reason for login failure is that we must install a + // required update, we definitely want to pass control to the + // updater to manage that for us. We'll handle any other login + // failure ourselves, as usual. We figure that no matter where you + // are in the world, or what kind of network you're on, we can + // reasonably expect the Viewer Version Manager to respond more or + // less as quickly as login.cgi. This synchronization is only + // intended to smooth out minor races between the two services. + // But what if the updater crashes? Use a timeout so that + // eventually we'll tire of waiting for it and carry on as usual. + // Given the above, it can be a fairly short timeout, at least + // from a human point of view. + + // Since sSyncPoint is an LLEventMailDrop, we DEFINITELY want to + // consume the posted event. + LLCoros::OverrideConsuming oc(true); + // Timeout should produce the isUndefined() object passed here. + LL_DEBUGS("LLLogin") << "Login failure, waiting for sync from updater" << LL_ENDL; + LLSD updater = llcoro::suspendUntilEventOnWithTimeout(sSyncPoint, 10, LLSD()); + if (updater.isUndefined()) + { + LL_WARNS("LLLogin") << "Failed to hear from updater, proceeding with fail.login" + << LL_ENDL; + } + else + { + LL_DEBUGS("LLLogin") << "Got responses from updater and login.cgi" << LL_ENDL; + } + // Let the fail.login handler deal with empty updater response. + LLSD responses(mAuthResponse["responses"]); + responses["updater"] = updater; + sendProgressEvent("offline", "fail.login", responses); } - // Let the fail.login handler deal with empty updater response. - LLSD responses(mAuthResponse["responses"]); - responses["updater"] = updater; - sendProgressEvent("offline", "fail.login", responses); + return; // Done! } - return; // Done! - } -// /* Sometimes we end with "Started" here. Slightly slow server? -// * Seems to be ok to just skip it. Otherwise we'd error out and crash in the if below. -// */ -// if( status == "Started") -// { -// LL_DEBUGS("LLLogin") << mAuthResponse << LL_ENDL; -// continue; -// } - - // If we don't recognize status at all, trouble - if (! (status == "CURLError" - || status == "XMLRPCError" - || status == "OtherError")) - { - LL_ERRS("LLLogin") << "Unexpected status from " << xmlrpcPump.getName() << " pump: " - << mAuthResponse << LL_ENDL; - return; - } +/*==========================================================================*| + // Sometimes we end with "Started" here. Slightly slow server? Seems + // to be ok to just skip it. Otherwise we'd error out and crash in the + // if below. + if( status == "Started") + { + LL_DEBUGS("LLLogin") << mAuthResponse << LL_ENDL; + continue; + } +|*==========================================================================*/ - // Here status IS one of the errors tested above. - // Tell caller this didn't work out so well. - - // *NOTE: The response from LLXMLRPCListener's Poller::poll method returns an - // llsd with no "responses" node. To make the output from an incomplete login symmetrical - // to success, add a data/message and data/reason fields. - LLSD error_response(LLSDMap - ("reason", mAuthResponse["status"]) - ("errorcode", mAuthResponse["errorcode"]) - ("message", mAuthResponse["error"])); - if(mAuthResponse.has("certificate")) - { - error_response["certificate"] = mAuthResponse["certificate"]; - } - sendProgressEvent("offline", "fail.login", error_response); + // If we don't recognize status at all, trouble + if (! (status == "CURLError" + || status == "XMLRPCError" + || status == "OtherError")) + { + LL_ERRS("LLLogin") << "Unexpected status from " << xmlrpcPump.getName() << " pump: " + << mAuthResponse << LL_ENDL; + return; + } + + // Here status IS one of the errors tested above. + // Tell caller this didn't work out so well. + + // *NOTE: The response from LLXMLRPCListener's Poller::poll method returns an + // llsd with no "responses" node. To make the output from an incomplete login symmetrical + // to success, add a data/message and data/reason fields. + LLSD error_response(LLSDMap + ("reason", mAuthResponse["status"]) + ("errorcode", mAuthResponse["errorcode"]) + ("message", mAuthResponse["error"])); + if(mAuthResponse.has("certificate")) + { + error_response["certificate"] = mAuthResponse["certificate"]; + } + sendProgressEvent("offline", "fail.login", error_response); } catch (...) { - CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName() - << "('" << uri << "', " << printable_params << ")")); + LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName() + << "('" << uri << "', " << printable_params << ")")); + throw; } } -- cgit v1.3