Skip to content
Snippets Groups Projects
Commit 87da08b1 authored by Nat Goodspeed's avatar Nat Goodspeed
Browse files

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.
parent 5ab0ff48
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
indra/llcommon/llcoros.h
+ 5
− 0
View file @ 87da08b1
...@@ -232,6 +232,11 @@ class LL_COMMON_API LLCoros: public LLSingleton<LLCoros> ...@@ -232,6 +232,11 @@ class LL_COMMON_API LLCoros: public LLSingleton<LLCoros>
}; };
/// thrown by checkStop() /// 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 struct Stop: public LLContinueError
{ {
Stop(const std::string& what): LLContinueError(what) {} Stop(const std::string& what): LLContinueError(what) {}
......
indra/viewer_components/login/lllogin.cpp
+ 143
− 140
View file @ 87da08b1
...@@ -148,167 +148,170 @@ void LLLogin::Impl::loginCoro(std::string uri, LLSD login_params) ...@@ -148,167 +148,170 @@ void LLLogin::Impl::loginCoro(std::string uri, LLSD login_params)
} }
try try
{ {
LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::getName() LL_DEBUGS("LLLogin") << "Entering coroutine " << LLCoros::getName()
<< " with uri '" << uri << "', parameters " << printable_params << LL_ENDL; << " with uri '" << uri << "', parameters " << printable_params << LL_ENDL;
LLEventPump& xmlrpcPump(LLEventPumps::instance().obtain("LLXMLRPCTransaction")); LLEventPump& xmlrpcPump(LLEventPumps::instance().obtain("LLXMLRPCTransaction"));
// EXT-4193: use a DIFFERENT reply pump than for the SRV request. We used // 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 // 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. // SRV response to arrive just as we were expecting the XMLRPC response.
LLEventStream loginReplyPump("loginreply", true); LLEventStream loginReplyPump("loginreply", true);
LLSD::Integer attempts = 0; LLSD::Integer attempts = 0;
LLSD request(login_params); LLSD request(login_params);
request["reply"] = loginReplyPump.getName(); request["reply"] = loginReplyPump.getName();
request["uri"] = uri; request["uri"] = uri;
std::string status; std::string status;
// Loop back to here if login attempt redirects to a different // Loop back to here if login attempt redirects to a different
// request["uri"] // request["uri"]
for (;;) 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)))
{ {
// Still Downloading -- send progress update. ++attempts;
sendProgressEvent("offline", "downloading"); 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; LL_DEBUGS("LLLogin") << "Auth Response: " << mAuthResponse << LL_ENDL;
status = mAuthResponse["status"].asString(); status = mAuthResponse["status"].asString();
// Okay, we've received our final status event for this // Okay, we've received our final status event for this
// request. Unless we got a redirect response, break the retry // request. Unless we got a redirect response, break the retry
// loop for the current rewrittenURIs entry. // loop for the current rewrittenURIs entry.
if (!(status == "Complete" && if (!(status == "Complete" &&
mAuthResponse["responses"]["login"].asString() == "indeterminate")) mAuthResponse["responses"]["login"].asString() == "indeterminate"))
{ {
break; break;
} }
sendProgressEvent("offline", "indeterminate", mAuthResponse["responses"]); sendProgressEvent("offline", "indeterminate", mAuthResponse["responses"]);
// Here the login service at the current URI is redirecting us // Here the login service at the current URI is redirecting us
// to some other URI ("indeterminate" -- why not "redirect"?). // to some other URI ("indeterminate" -- why not "redirect"?).
// The response should contain another uri to try, with its // The response should contain another uri to try, with its
// own auth method. // own auth method.
request["uri"] = mAuthResponse["responses"]["next_url"].asString(); request["uri"] = mAuthResponse["responses"]["next_url"].asString();
request["method"] = mAuthResponse["responses"]["next_method"].asString(); request["method"] = mAuthResponse["responses"]["next_method"].asString();
} // loop back to try the redirected URI } // loop back to try the redirected URI
// Here we're done with redirects. // Here we're done with redirects.
if (status == "Complete") 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
{ {
// Synchronize here with the updater. We synchronize here rather // StatusComplete does not imply auth success. Check the
// than in the fail.login handler, which actually examines the // actual outcome of the request. We've already handled the
// response from login.cgi, because here we are definitely in a // "indeterminate" case in the loop above.
// coroutine and can definitely use suspendUntilBlah(). Whoever's if (mAuthResponse["responses"]["login"].asString() == "true")
// 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" sendProgressEvent("online", "connect", mAuthResponse["responses"]);
<< LL_ENDL;
} }
else 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. return; // Done!
LLSD responses(mAuthResponse["responses"]);
responses["updater"] = updater;
sendProgressEvent("offline", "fail.login", responses);
} }
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. // 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( status == "Started") // if below.
// { if( status == "Started")
// LL_DEBUGS("LLLogin") << mAuthResponse << LL_ENDL; {
// continue; 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;
}
// Here status IS one of the errors tested above. // If we don't recognize status at all, trouble
// Tell caller this didn't work out so well. if (! (status == "CURLError"
|| status == "XMLRPCError"
// *NOTE: The response from LLXMLRPCListener's Poller::poll method returns an || status == "OtherError"))
// llsd with no "responses" node. To make the output from an incomplete login symmetrical {
// to success, add a data/message and data/reason fields. LL_ERRS("LLLogin") << "Unexpected status from " << xmlrpcPump.getName() << " pump: "
LLSD error_response(LLSDMap << mAuthResponse << LL_ENDL;
("reason", mAuthResponse["status"]) return;
("errorcode", mAuthResponse["errorcode"]) }
("message", mAuthResponse["error"]));
if(mAuthResponse.has("certificate")) // Here status IS one of the errors tested above.
{ // Tell caller this didn't work out so well.
error_response["certificate"] = mAuthResponse["certificate"];
} // *NOTE: The response from LLXMLRPCListener's Poller::poll method returns an
sendProgressEvent("offline", "fail.login", error_response); // 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 (...) { catch (...) {
CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName() LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << LLCoros::getName()
<< "('" << uri << "', " << printable_params << ")")); << "('" << uri << "', " << printable_params << ")"));
throw;
} }
} }
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment