Auto merge of #9358 - jsanders:refactor-websocket-closing, r=jdm

Clean up websocket closing code

Fixes #7860.

This also changes quite a bit about how close codes are implemented, I believe bringing them closer in line with the spec. Instead of saving off the close code sent by the client, it uses the code from the server's closing handshake. It also handles `NO_STATUS` in what I believe is the correct manner. Making this work required a change to the test harness to make the `/echo` websocket handler echo the code sent by the client and handle `NO_STATUS` properly, rather than always replying with `NORMAL`.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9358)
<!-- Reviewable:end -->

components/net/websocket_loader.rs

@@ -71,7 +71,7 @@ pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData) {
             Ok(net_url) => net_url,
             Err(e) => {
                 debug!("Failed to establish a WebSocket connection: {:?}", e);
-                let _ = connect.event_sender.send(WebSocketNetworkEvent::Close);
+                let _ = connect.event_sender.send(WebSocketNetworkEvent::Fail);
                 return;
             }
         };
@@ -87,7 +87,7 @@ pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData) {
             },
             Err(e) => {
                 debug!("Failed to establish a WebSocket connection: {:?}", e);
-                let _ = connect.event_sender.send(WebSocketNetworkEvent::Close);
+                let _ = connect.event_sender.send(WebSocketNetworkEvent::Fail);
                 return;
             }
 
@@ -114,7 +114,9 @@ pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData) {
                     Type::Pong => continue,
                     Type::Close => {
                         ws_sender_incoming.lock().unwrap().send_message(&message).unwrap();
-                        let _ = resource_event_sender.send(WebSocketNetworkEvent::Close);
+                        let code = message.cd_status_code;
+                        let reason = String::from_utf8_lossy(&message.payload).into_owned();
+                        let _ = resource_event_sender.send(WebSocketNetworkEvent::Close(code, reason));
                         break;
                     },
                 };
@@ -134,8 +136,11 @@ pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData) {
                         ws_sender_outgoing.lock().unwrap().send_message(&Message::binary(data)).unwrap();
                     },
                     WebSocketDomAction::Close(code, reason) => {
-                        ws_sender_outgoing.lock().unwrap()
+                        let message = match code {
-                        .send_message(&Message::close_because(code, reason)).unwrap();
+                            Some(code) => Message::close_because(code, reason.unwrap_or("".to_owned())),
+                            None => Message::close()
+                        };
+                        ws_sender_outgoing.lock().unwrap().send_message(&message).unwrap();
                     },
                 }
             }

components/net_traits/lib.rs

@@ -178,14 +178,15 @@ pub enum MessageData {
 #[derive(Deserialize, Serialize)]
 pub enum WebSocketDomAction {
     SendMessage(MessageData),
-    Close(u16, String),
+    Close(Option<u16>, Option<String>),
 }
 
 #[derive(Deserialize, Serialize)]
 pub enum WebSocketNetworkEvent {
     ConnectionEstablished(header::Headers, Vec<String>),
     MessageReceived(MessageData),
-    Close,
+    Close(Option<u16>, String),
+    Fail,
 }
 
 #[derive(Deserialize, Serialize)]

components/script/dom/websocket.rs

@@ -33,7 +33,7 @@ use net_traits::hosts::replace_hosts;
 use net_traits::unwrap_websocket_protocol;
 use net_traits::{WebSocketCommunicate, WebSocketConnectData, WebSocketDomAction, WebSocketNetworkEvent};
 use script_thread::ScriptThreadEventCategory::WebSocketEvent;
-use script_thread::{CommonScriptMsg, Runnable};
+use script_thread::{CommonScriptMsg, Runnable, ScriptChan};
 use std::borrow::ToOwned;
 use std::cell::Cell;
 use std::ptr;
@@ -132,6 +132,29 @@ mod close_code {
     pub const TLS_FAILED: u16 = 1015;
 }
 
+pub fn close_the_websocket_connection(address: Trusted<WebSocket>,
+                                      sender: Box<ScriptChan>,
+                                      code: Option<u16>,
+                                      reason: String) {
+    let close_task = box CloseTask {
+        addr: address,
+        failed: false,
+        code: code,
+        reason: Some(reason),
+    };
+    sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, close_task)).unwrap();
+}
+
+pub fn fail_the_websocket_connection(address: Trusted<WebSocket>, sender: Box<ScriptChan>) {
+    let close_task = box CloseTask {
+        addr: address,
+        failed: true,
+        code: Some(close_code::ABNORMAL),
+        reason: None,
+    };
+    sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, close_task)).unwrap();
+}
+
 #[dom_struct]
 pub struct WebSocket {
     eventtarget: EventTarget,
@@ -141,11 +164,6 @@ pub struct WebSocket {
     clearing_buffer: Cell<bool>, //Flag to tell if there is a running thread to clear buffered_amount
     #[ignore_heap_size_of = "Defined in std"]
     sender: DOMRefCell<Option<IpcSender<WebSocketDomAction>>>,
-    failed: Cell<bool>, //Flag to tell if websocket was closed due to failure
-    full: Cell<bool>, //Flag to tell if websocket queue is full
-    clean_close: Cell<bool>, //Flag to tell if the websocket closed cleanly (not due to full or fail)
-    code: Cell<u16>, //Closing code
-    reason: DOMRefCell<String>, //Closing reason
     binary_type: Cell<BinaryType>,
     protocol: DOMRefCell<String>, //Subprotocol selected by server
 }
@@ -158,12 +176,7 @@ impl WebSocket {
             ready_state: Cell::new(WebSocketRequestState::Connecting),
             buffered_amount: Cell::new(0),
             clearing_buffer: Cell::new(false),
-            failed: Cell::new(false),
             sender: DOMRefCell::new(None),
-            full: Cell::new(false),
-            clean_close: Cell::new(true),
-            code: Cell::new(0),
-            reason: DOMRefCell::new("".to_owned()),
             binary_type: Cell::new(BinaryType::Blob),
             protocol: DOMRefCell::new("".to_owned()),
         }
@@ -272,11 +285,11 @@ impl WebSocket {
                         };
                         sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, message_thread)).unwrap();
                     },
-                    WebSocketNetworkEvent::Close => {
+                    WebSocketNetworkEvent::Fail => {
-                        let thread = box CloseTask {
+                        fail_the_websocket_connection(moved_address.clone(), sender.clone());
-                            addr: moved_address.clone(),
+                    },
-                        };
+                    WebSocketNetworkEvent::Close(code, reason) => {
-                        sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, thread)).unwrap();
+                        close_the_websocket_connection(moved_address.clone(), sender.clone(), code, reason);
                     },
                 }
             }
@@ -402,18 +415,6 @@ impl WebSocketMethods for WebSocket {
 
     // https://html.spec.whatwg.org/multipage/#dom-websocket-close
     fn Close(&self, code: Option<u16>, reason: Option<USVString>) -> Fallible<()>{
-        fn send_close(this: &WebSocket) {
-            this.ready_state.set(WebSocketRequestState::Closing);
-
-            let mut sender = this.sender.borrow_mut();
-            //TODO: Also check if the buffer is full
-            if let Some(sender) = sender.as_mut() {
-                let code: u16 = this.code.get();
-                let reason = this.reason.borrow().clone();
-                let _ = sender.send(WebSocketDomAction::Close(code, reason));
-            }
-        }
-
         if let Some(code) = code {
             //Fail if the supplied code isn't normal and isn't reserved for libraries, frameworks, and applications
             if code != close_code::NORMAL && (code < 3000 || code > 4999) {
@@ -431,21 +432,22 @@ impl WebSocketMethods for WebSocket {
             WebSocketRequestState::Connecting => { //Connection is not yet established
                 /*By setting the state to closing, the open function
                   will abort connecting the websocket*/
-                self.failed.set(true);
+                self.ready_state.set(WebSocketRequestState::Closing);
-                send_close(self);
+
-                //Note: After sending the close message, the receive loop confirms a close message from the server and
+                let global = self.global();
-                //      must fire a close event
+                let sender = global.r().networking_thread_source();
+                let address = Trusted::new(self, sender.clone());
+                fail_the_websocket_connection(address, sender);
             }
             WebSocketRequestState::Open => {
-                //Closing handshake not started - still in open
+                self.ready_state.set(WebSocketRequestState::Closing);
-                //Start the closing by setting the code and reason if they exist
+
-                self.code.set(code.unwrap_or(close_code::NO_STATUS));
+                // Kick off _Start the WebSocket Closing Handshake_
-                if let Some(reason) = reason {
+                // https://tools.ietf.org/html/rfc6455#section-7.1.2
-                    *self.reason.borrow_mut() = reason.0;
+                let reason = reason.map(|reason| reason.0);
-                }
+                let mut other_sender = self.sender.borrow_mut();
-                send_close(self);
+                let my_sender = other_sender.as_mut().unwrap();
-                //Note: After sending the close message, the receive loop confirms a close message from the server and
+                let _ = my_sender.send(WebSocketDomAction::Close(code, reason));
-                //      must fire a close event
             }
         }
         Ok(()) //Return Ok
@@ -467,13 +469,8 @@ impl Runnable for ConnectionEstablishedTask {
 
         // Step 1: Protocols.
         if !self.protocols.is_empty() && self.headers.get::<WebSocketProtocol>().is_none() {
-            ws.failed.set(true);
-            ws.ready_state.set(WebSocketRequestState::Closing);
-            let thread = box CloseTask {
-                addr: self.addr,
-            };
             let sender = global.r().networking_thread_source();
-            sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, thread)).unwrap();
+            fail_the_websocket_connection(self.addr, sender);
             return;
         }
 
@@ -516,6 +513,9 @@ impl Runnable for BufferedAmountTask {
 
 struct CloseTask {
     addr: Trusted<WebSocket>,
+    failed: bool,
+    code: Option<u16>,
+    reason: Option<String>,
 }
 
 impl Runnable for CloseTask {
@@ -523,29 +523,37 @@ impl Runnable for CloseTask {
         let ws = self.addr.root();
         let ws = ws.r();
         let global = ws.global();
+
+        if ws.ready_state.get() == WebSocketRequestState::Closed {
+            // Do nothing if already closed.
+            return;
+        }
+
+        // Perform _the WebSocket connection is closed_ steps.
+        // https://html.spec.whatwg.org/multipage/#closeWebSocket
+
+        // Step 1.
         ws.ready_state.set(WebSocketRequestState::Closed);
-        //If failed or full, fire error event
+
-        if ws.failed.get() || ws.full.get() {
+        // Step 2.
-            ws.failed.set(false);
+        if self.failed {
-            ws.full.set(false);
-            //A Bad close
-            ws.clean_close.set(false);
             ws.upcast().fire_event("error",
                                    EventBubbles::DoesNotBubble,
                                    EventCancelable::Cancelable,
                                    global.r());
         }
-        let reason = ws.reason.borrow().clone();
+
-        /*In addition, we also have to fire a close even if error event fired
+        // Step 3.
-         https://html.spec.whatwg.org/multipage/#closeWebSocket
+        let clean_close = !self.failed;
-        */
+        let code = self.code.unwrap_or(close_code::NO_STATUS);
+        let reason = DOMString::from(self.reason.unwrap_or("".to_owned()));
         let close_event = CloseEvent::new(global.r(),
                                           atom!("close"),
                                           EventBubbles::DoesNotBubble,
                                           EventCancelable::NotCancelable,
-                                          ws.clean_close.get(),
+                                          clean_close,
-                                          ws.code.get(),
+                                          code,
-                                          DOMString::from(reason));
+                                          reason);
         close_event.upcast::<Event>().fire(ws.upcast());
     }
 }

tests/wpt/metadata/websockets/interfaces/WebSocket/events/020.html.ini

@@ -1,6 +0,0 @@
-[020.html]
-  type: testharness
-  expected: TIMEOUT
-  [WebSockets: error events]
-    expected: TIMEOUT
-

tests/wpt/metadata/websockets/opening-handshake/001.html.ini

@@ -1,5 +0,0 @@
-[001.html]
-  type: testharness
-  [WebSockets: invalid handshake]
-    expected: FAIL
-

tests/wpt/web-platform-tests/websockets/handlers/echo_wsh.py

@@ -1,5 +1,6 @@
 #!/usr/bin/python
 from mod_pywebsocket import msgutil
+from mod_pywebsocket import common
 
 _GOODBYE_MESSAGE = u'Goodbye'
 
@@ -23,3 +24,13 @@ def web_socket_transfer_data(request):
         else:
             request.ws_stream.send_message(line, binary=True)
 
+def web_socket_passive_closing_handshake(request):
+    # Echo close status code and reason
+    code, reason = request.ws_close_code, request.ws_close_reason
+
+    # No status received is a reserved pseudo code representing an empty code,
+    # so echo back an empty code in this case.
+    if code == common.STATUS_NO_STATUS_RECEIVED:
+        code = None
+
+    return code, reason