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 -->

6 files changed, 92 insertions, 78 deletions

diff --git a/components/net/websocket_loader.rs b/components/net/websocket_loader.rs
index 461972b35f0..e25d7e7389f 100644
--- a/components/net/websocket_loader.rs
+++ b/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()
-                        .send_message(&Message::close_because(code, reason)).unwrap();
+                        let message = match code {
+                            Some(code) => Message::close_because(code, reason.unwrap_or("".to_owned())),
+                            None => Message::close()
+                        };
+                        ws_sender_outgoing.lock().unwrap().send_message(&message).unwrap();
                     },
                 }
             }
diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs
index 5830f115514..654a64e811c 100644
--- a/components/net_traits/lib.rs
+++ b/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)]
diff --git a/components/script/dom/websocket.rs b/components/script/dom/websocket.rs
index 031b66cb8d8..36456cd02ff 100644
--- a/components/script/dom/websocket.rs
+++ b/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 => {
-                        let thread = box CloseTask {
-                            addr: moved_address.clone(),
-                        };
-                        sender.send(CommonScriptMsg::RunnableMsg(WebSocketEvent, thread)).unwrap();
+                    WebSocketNetworkEvent::Fail => {
+                        fail_the_websocket_connection(moved_address.clone(), sender.clone());
+                    },
+                    WebSocketNetworkEvent::Close(code, reason) => {
+                        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);
-                send_close(self);
-                //Note: After sending the close message, the receive loop confirms a close message from the server and
-                //      must fire a close event
+                self.ready_state.set(WebSocketRequestState::Closing);
+
+                let global = self.global();
+                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
-                //Start the closing by setting the code and reason if they exist
-                self.code.set(code.unwrap_or(close_code::NO_STATUS));
-                if let Some(reason) = reason {
-                    *self.reason.borrow_mut() = reason.0;
-                }
-                send_close(self);
-                //Note: After sending the close message, the receive loop confirms a close message from the server and
-                //      must fire a close event
+                self.ready_state.set(WebSocketRequestState::Closing);
+
+                // Kick off _Start the WebSocket Closing Handshake_
+                // https://tools.ietf.org/html/rfc6455#section-7.1.2
+                let reason = reason.map(|reason| reason.0);
+                let mut other_sender = self.sender.borrow_mut();
+                let my_sender = other_sender.as_mut().unwrap();
+                let _ = my_sender.send(WebSocketDomAction::Close(code, reason));
             }
         }
         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() {
-            ws.failed.set(false);
-            ws.full.set(false);
-            //A Bad close
-            ws.clean_close.set(false);
+
+        // Step 2.
+        if self.failed {
             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
-         https://html.spec.whatwg.org/multipage/#closeWebSocket
-        */
+
+        // Step 3.
+        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(),
-                                          ws.code.get(),
-                                          DOMString::from(reason));
+                                          clean_close,
+                                          code,
+                                          reason);
         close_event.upcast::<Event>().fire(ws.upcast());
     }
 }
diff --git a/tests/wpt/metadata/websockets/interfaces/WebSocket/events/020.html.ini b/tests/wpt/metadata/websockets/interfaces/WebSocket/events/020.html.ini
deleted file mode 100644
index e12d8871a89..00000000000
--- a/tests/wpt/metadata/websockets/interfaces/WebSocket/events/020.html.ini
+++ /dev/null
@@ -1,6 +0,0 @@
-[020.html]
-  type: testharness
-  expected: TIMEOUT
-  [WebSockets: error events]
-    expected: TIMEOUT
-
diff --git a/tests/wpt/metadata/websockets/opening-handshake/001.html.ini b/tests/wpt/metadata/websockets/opening-handshake/001.html.ini
deleted file mode 100644
index 42dc79e1c4f..00000000000
--- a/tests/wpt/metadata/websockets/opening-handshake/001.html.ini
+++ /dev/null
@@ -1,5 +0,0 @@
-[001.html]
-  type: testharness
-  [WebSockets: invalid handshake]
-    expected: FAIL
-
diff --git a/tests/wpt/web-platform-tests/websockets/handlers/echo_wsh.py b/tests/wpt/web-platform-tests/websockets/handlers/echo_wsh.py
index 5829c2f8c18..41bcd169422 100755
--- a/tests/wpt/web-platform-tests/websockets/handlers/echo_wsh.py
+++ b/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