Fixing a potential AV issue with websocket winrt client if the task returned from close wasn't waited on before the destructor executes.

Author: stgates

Release/src/websockets/client/ws_client_winrt.cpp

@@ -85,6 +85,7 @@ class winrt_callback_client : public websocket_client_callback_impl, public std:
 public:
     winrt_callback_client(websocket_client_config config) :
         websocket_client_callback_impl(std::move(config)),
+        m_connected(false),
         m_num_sends(0)
     {
         m_msg_websocket = ref new MessageWebSocket();
@@ -144,23 +145,26 @@ class winrt_callback_client : public websocket_client_callback_impl, public std:
 
     ~winrt_callback_client()
     {
-        // Locally copy the task completion event since there is a PPL bug
-        // that the set method accesses internal state in the event and the websocket
-        // client could be destroyed.
-        auto local_close_tce = m_close_tce;
-        local_close_tce.set();
+        // Only call close if successfully connected.
+        if (m_connected)
+        {
+            // Users should have already called close and wait on the returned task
+            // before destroying the client. In case they didn't we call close and wait for
+            // it to complete. It is safe to call MessageWebSocket::Close multiple times and
+            // concurrently, it has safe guards in place to only execute once.
+            close().wait();
+        }
     }
 
     pplx::task<void> connect()
     {
+        _ASSERTE(!m_connected);
         const auto &proxy = m_config.proxy();
         if(!proxy.is_default())
         {
             return pplx::task_from_exception<void>(websocket_exception("Only a default proxy server is supported."));
         }
 
-
-
         const auto &proxy_cred = proxy.credentials();
         if(proxy_cred.is_set())
         {
@@ -187,6 +191,7 @@ class winrt_callback_client : public websocket_client_callback_impl, public std:
                 websocket_exception exc(e->HResult, build_error_msg(e, "ConnectAsync"));
                 return pplx::task_from_exception<void>(exc);
             }
+            m_connected = true;
             return pplx::task_from_result();
         });
     }
@@ -410,6 +415,9 @@ class winrt_callback_client : public websocket_client_callback_impl, public std:
 
     pplx::task_completion_event<void> m_close_tce;
 
+    // Tracks whether or not the websocket client successfully connected to the server.
+    std::atomic<bool> m_connected;
+
     // External callback for handling received and close event
     std::function<void(websocket_incoming_message)> m_external_message_handler;
     std::function<void(websocket_close_status, const utility::string_t&, const std::error_code&)> m_external_close_handler;

Release/tests/functional/websockets/client/error_tests.cpp

@@ -143,24 +143,12 @@ TEST_FIXTURE(uri_address, try_receive_after_server_initiated_close)
 }
 
 // Destroy the client without closing it explicitly
-// CodePlex 319 fails on VS2013.
-#if !defined(_MSC_VER) || _MSC_VER >= 1900
 TEST_FIXTURE(uri_address, destroy_without_close)
 {
     test_websocket_server server;
-    pplx::task<websocket_incoming_message> t;
-
-    {
-        websocket_client client;
-
-        client.connect(m_uri).wait();
-
-        t = client.receive();
-    }
-    
-    VERIFY_THROWS(t.wait(), websocket_exception);
+    websocket_client client;
+    client.connect(m_uri).wait();
 }
-#endif
 
 // Destroy the callback client without closing it explicitly
 TEST_FIXTURE(uri_address, destroy_without_close_callback_client)