Closed drunderscore closed 3 years ago
With all the debug macros,
This is what it looks like when it is triggered multiple times (incorrect)
What it looks like when triggered once (correct):
I believe the following patch would be a correct solution:
diff --git a/Userland/Libraries/LibTLS/Socket.cpp b/Userland/Libraries/LibTLS/Socket.cpp
index 92c7afe1e..041eb1f8d 100644
--- a/Userland/Libraries/LibTLS/Socket.cpp
+++ b/Userland/Libraries/LibTLS/Socket.cpp
@@ -186,6 +186,7 @@ bool TLSv12::check_connection_state(bool read)
// an abrupt closure (the server is a jerk)
dbgln_if(TLS_DEBUG, "Socket not open, assuming abrupt closure");
m_context.connection_finished = true;
+ return false;
}
if (m_context.critical_error) {
dbgln_if(TLS_DEBUG, "CRITICAL ERROR {} :(", m_context.critical_error);
it's pretty odd for that to call finished
multiple times, since this bit should guard that:
// Next iteration: m_context.connection_finished is false, so we don't even go in here...?
if (((read && m_context.application_buffer.size() == 0) || !read) && m_context.connection_finished) {
/// m_context.connection_finished must be true here
if (m_context.application_buffer.size() == 0) {
// m_context.application_buffer.size() must be zero here
if (on_tls_finished)
on_tls_finished();
}
if (m_context.tls_buffer.size()) {
dbgln_if(TLS_DEBUG, "connection closed without finishing data transfer, {} bytes still in buffer and {} bytes in application buffer",
m_context.tls_buffer.size(),
m_context.application_buffer.size());
} else {
// m_context.application_buffer.size() must be zero here
// This is likely the bug: vvvvv
m_context.connection_finished = false;
dbgln_if(TLS_DEBUG, "FINISHED");
}
if (!m_context.application_buffer.size()) {
// m_context.application_buffer.size() must be zero here
// m_context.connection_finished is set to zero by the branch above
m_context.connection_status = ConnectionStatus::Disconnected;
return false;
}
}
But if your patch fixes the weirdness, just set m_context.connection_status
and return false :shrug:
Try setting the false marked by vvvvv to true, that might be the problem...
Oh...
That should set connection_finished
to true, not false...
no, it does, that's the original code, no?
Oh I see what you mean,
m_context.connection_finished = false;
dbgln_if(TLS_DEBUG, "FINISHED");
thats an oopsie!
Or...hmmm That has to be false for us not to enter this whole if 🤔
3am logic isn't good, sorry
When just setting the above mistake to true doesn't fix this issue's problem, but it's still wrong so :^)
With the following code,
on_finish
will trigger multiple times, very randomly, but usually only ever when hit with a non-successful response code:This is being built on Lagom:
Here's what the bug being triggered looks like: https://user-images.githubusercontent.com/15949431/122999893-d0f42a00-d37c-11eb-99c7-16c63ac4bd10.mp4