Improve redirect behaviour to clear headers and reevaluate sent cookies. Implement storage-related cookie behaviour such as domain and path matching that cookie-rs doesn't require. Remove stored cookies when an empty value is stored. Document cookie code.

components/net/Cargo.toml

@@ -27,3 +27,4 @@ git = "https://github.com/servo/rust-stb-image"
 url = "0.2.16"
 time = "0.1.12"
 openssl="0.2.15"
+cookie = "*"

components/net/about_loader.rs

@@ -3,7 +3,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use resource_task::{TargetedLoadResponse, Metadata, LoadData, start_sending, ResponseSenders};
-use resource_task::ControlMsg;
 use resource_task::ProgressMsg::Done;
 use file_loader;
 
@@ -15,7 +14,7 @@ use std::borrow::ToOwned;
 use std::io::fs::PathExtensions;
 use std::sync::mpsc::Sender;
 
-pub fn factory(mut load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+pub fn factory(mut load_data: LoadData, start_chan: Sender<TargetedLoadResponse>) {
     let senders = ResponseSenders {
         immediate_consumer: start_chan.clone(),
         eventual_consumer: load_data.consumer.clone(),
@@ -28,8 +27,7 @@ pub fn factory(mut load_data: LoadData, start_chan: Sender<TargetedLoadResponse>
                 charset: Some("utf-8".to_string()),
                 headers: None,
                 status: Some(RawStatus(200, "OK".to_owned())),
-                cookies: Vec::new(),
+            });
-            }, cookies_chan);
             chan.send(Done(Ok(()))).unwrap();
             return
         }
@@ -41,10 +39,10 @@ pub fn factory(mut load_data: LoadData, start_chan: Sender<TargetedLoadResponse>
             load_data.url = Url::from_file_path(&path).unwrap();
         }
         _ => {
-            start_sending(senders, Metadata::default(load_data.url), cookies_chan)
+            start_sending(senders, Metadata::default(load_data.url))
                 .send(Done(Err("Unknown about: URL.".to_string()))).unwrap();
             return
         }
     };
-    file_loader::factory(load_data, start_chan, cookies_chan)
+    file_loader::factory(load_data, start_chan)
 }

components/net/cookie.rs

@@ -2,206 +2,122 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-//use resource_task::{Metadata, Payload, Done, LoadResponse, LoaderTask, start_sending};
+//! Implementation of cookie creation and matching as specified by
-//use time::Tm;
+//! http://tools.ietf.org/html/rfc6265
-use url::Url;
-use std::ascii::AsciiExt;
-use time::{strptime, Tm, at, get_time, Timespec, now};
-use std::i64;
-use pub_domains::PUB_DOMAINS;
-use std::io::net::ip::IpAddr;
 
+use pub_domains::PUB_DOMAINS;
+
+use cookie_rs;
+use time::{Tm, now, at, Timespec};
+use url::Url;
+use std::borrow::ToOwned;
+use std::i64;
+use std::io::net::ip::IpAddr;
+use std::time::Duration;
+
+/// A stored cookie that wraps the definition in cookie-rs. This is used to implement
+/// various behaviours defined in the spec that rely on an associated request URL,
+/// which cookie-rs and hyper's header parsing do not support.
 #[derive(Clone, Show)]
 pub struct Cookie {
-    pub name: String,
+    pub cookie: cookie_rs::Cookie,
-    pub value: String,
-    pub domain: String,
-    pub expires: Option<Tm>,
-    pub path: String,
-    pub secure: bool,
-    pub http_only: bool,
     pub host_only: bool,
     pub persistent: bool,
     pub created_at: Tm,
     pub last_access: Tm,
-    pub scheme: String
+    pub scheme: String,
+    pub expiry_time: Tm,
 }
 
 impl Cookie {
-    pub fn new(header_value: String, url: &Url) -> Option<Cookie> {
+    /// http://tools.ietf.org/html/rfc6265#section-5.3
-        let mut secure = false;
+    pub fn new_wrapped(mut cookie: cookie_rs::Cookie, request: &Url) -> Option<Cookie> {
-        let mut http_only = false;
+        // Step 3
-        let mut name = None;
+        let (persistent, expiry_time) = match (&cookie.max_age, &cookie.expires) {
-        let mut value = None;
+            (&Some(max_age), _) => (true, at(now().to_timespec() + Duration::seconds(max_age as i64))),
-        let mut domain = "".to_string();
+            (_, &Some(expires)) => (true, expires),
-        let mut path = "".to_string();
+            _ => (false, at(Timespec::new(i64::MAX, 0)))
-        let mut expires = None;
-        let mut max_age = None;
-
-        let mut set_cookie_iter = header_value.as_slice().split(';');
-        match set_cookie_iter.next() {
-            Some(name_value_pair) => {
-                if !name_value_pair.contains_char('=') {
-                    return None;
-                }
-                let mut data = name_value_pair.split('=').map(|x| x.trim());
-                name = data.next();
-                value = data.next();
-            }
-            None => { return None }
-        }
-
-        if name.is_some() && name.unwrap() == "" {
-            return None
-        }
-
-        for spl in set_cookie_iter {
-            let cookie_av = spl.trim();
-            if cookie_av.contains_char('=') {
-                match cookie_av.split('=').map(|x| x.trim()).collect::<Vec<&str>>().as_slice() {
-                    [attr, val] if attr.eq_ignore_ascii_case("domain") => {
-                        if val == "" {
-                            continue;
-                        }
-                        let cookie_domain;
-                        if val.char_at(0) == '.' {
-                            cookie_domain = val.slice_from(1);
-                        } else {
-                            cookie_domain = val;
-                        }
-                        domain = cookie_domain.to_ascii_lowercase();
-                    }
-                    [attr, val] if attr.eq_ignore_ascii_case("path") => {
-                        if val == "" || val.char_at(0) != '/' {
-                            match url.path() {
-                                Some(x) => {
-                                    let mut url_path = "".to_string();
-                                    for v in x.iter() {
-                                        url_path.push_str(v.as_slice())
-                                    }
-                                    path = Cookie::default_path(url_path.as_slice())
-                                }
-                                _ => {
-                                    return None
-                                }
-                            }
-                        } else {
-                            path = val.to_string();
-                        }
-
-                    }
-                    [attr, val] if attr.eq_ignore_ascii_case("expires") => {
-                        // we try strptime with three date formats according to
-                        // http://tools.ietf.org/html/rfc2616#section-3.3.1
-                        match strptime(val, "%a, %d %b %Y %H:%M:%S %Z") {
-                            Ok(x) => expires = Some(x),
-                            Err(_) => {
-                                match strptime(val, "%A, %d-%b-%y %H:%M:%S %Z") {
-                                    Ok(x) => expires = Some(x),
-                                    Err(_) => {
-                                        match strptime(val, "%a %b %d %H:%M:%S %Y") {
-                                            Ok(x) => expires = Some(x),
-                                            Err(_) => continue
-                                        }
-                                    }
-                                }
-                            }
-                        }
-                    }
-                    [attr, val] if attr.eq_ignore_ascii_case("max-age") => {
-                        match val.parse() {
-                            Some(x) if x > 0 => {
-                                let mut expires = get_time();
-                                expires.sec += x;
-                                max_age = Some(at(expires));
-                            }
-                            Some(_) => {
-                                max_age = Some(at(Timespec::new(0, 0)))
-                            }
-                            None => continue
-                        }
-                    }
-                    x => { println!("Undefined cookie attr value: {:?}", x); }
-                }
-            } else if cookie_av.eq_ignore_ascii_case("secure") {
-                secure = true;
-            } else if cookie_av.eq_ignore_ascii_case("httponly") {
-                http_only = true;
-            } else {
-                println!("Undefined cookie attr value: {}", cookie_av)
-            }
-        }
-
-        let url_host = match url.host() {
-            Some(x) => x.serialize(),
-            None => "".to_string()
-        };
-        let mut cookie = Cookie {
-            name: name.unwrap().to_string(),
-            value: value.unwrap().to_string(),
-            created_at: now(),
-            last_access: now(),
-            domain: url_host.clone(),
-            expires: None,
-            path: path,
-            secure: secure,
-            http_only: http_only,
-            host_only: true,
-            persistent: false,
-            scheme: url.scheme.clone()
         };
 
-        if max_age.is_some() {
+        let url_host = request.host().map(|host| host.serialize()).unwrap_or("".to_owned());
-            cookie.persistent = true;
-            cookie.expires = max_age;
-        } else if expires.is_some() {
-            cookie.persistent = true;
-            cookie.expires = expires;
-        } else {
-            cookie.expires = Some(at(Timespec::new(i64::MAX, 0)))
-        }
 
+        // Step 4
+        let mut domain = cookie.domain.clone().unwrap_or("".to_owned());
+
+        // Step 5
         match PUB_DOMAINS.iter().find(|&x| domain == *x) {
             Some(val) if *val == url_host => domain = "".to_string(),
             Some(_) => return None,
             None => {}
         }
-        if !domain.is_empty() {
+
+        // Step 6
+        let host_only = if !domain.is_empty() {
             if !Cookie::domain_match(url_host.as_slice(), domain.as_slice()) {
                 return None;
             } else {
-                cookie.host_only = false;
+                cookie.domain = Some(domain);
-                cookie.domain = domain;
+                false
+            }
+        } else {
+            cookie.domain = Some(url_host);
+            true
+        };
+
+        // Step 7
+        let mut path = cookie.path.unwrap_or("".to_owned());
+        if path.is_empty() || path.char_at(0) != '/' {
+            let mut url_path: String = "".to_owned();
+            if let Some(paths) = request.path() {
+                for path in paths.iter() {
+                    url_path.extend(path.chars());
                 }
             }
-        if cookie.http_only && !url.scheme.as_slice().starts_with("http") {
+            path = Cookie::default_path(url_path.as_slice());
+        }
+        cookie.path = Some(path);
+
+
+        // Step 10
+        if cookie.httponly && !request.scheme.as_slice().starts_with("http") {
             return None;
         }
 
-        Some(cookie)
+        Some(Cookie {
+            cookie: cookie,
+            host_only: host_only,
+            persistent: persistent,
+            created_at: now(),
+            last_access: now(),
+            scheme: request.scheme.clone(),
+            expiry_time: expiry_time,
+        })
     }
 
     pub fn touch(&mut self) {
         self.last_access = now();
     }
 
+    // http://tools.ietf.org/html/rfc6265#section-5.1.4
     fn default_path(request_path: &str) -> String {
         if request_path == "" || request_path.char_at(0) != '/' || request_path == "/" {
-            return "/".to_string();
+            return "/".to_owned();
         }
         if request_path.ends_with("/") {
             return request_path.slice_to(request_path.len()-1).to_string();
         }
-        return request_path.clone().to_string();
+        return request_path.to_owned();
     }
 
+    // http://tools.ietf.org/html/rfc6265#section-5.1.4
     pub fn path_match(request_path: &str, cookie_path: &str) -> bool {
         request_path == cookie_path ||
         ( request_path.starts_with(cookie_path) &&
-            ( request_path.ends_with("/") || request_path.char_at(cookie_path.len()) == '/' )
+            ( request_path.ends_with("/") || request_path.char_at(cookie_path.len() - 1) == '/' )
         )
     }
 
+    // http://tools.ietf.org/html/rfc6265#section-5.1.3
     pub fn domain_match(string: &str, domain_string: &str) -> bool {
         if string == domain_string {
             return true;
@@ -214,22 +130,35 @@ impl Cookie {
         false
     }
 
+    // http://tools.ietf.org/html/rfc6265#section-5.4 step 1
     pub fn appropriate_for_url(&self, url: Url) -> bool {
-        let domain = url.host().unwrap().serialize();
+        let domain = url.host().map(|host| host.serialize());
-        let mut result = if self.host_only {
+        if self.host_only {
-            self.domain == domain
+            if self.cookie.domain != domain {
+                return false;
+            }
         } else {
-            Cookie::domain_match(domain.as_slice(), self.domain.as_slice())
+            if let (Some(ref domain), &Some(ref cookie_domain)) = (domain, &self.cookie.domain) {
-        };
+                if !Cookie::domain_match(domain.as_slice(), cookie_domain.as_slice()) {
-        result = result && Cookie::path_match(url.serialize_path().unwrap().as_slice(), self.path.as_slice());
+                    return false;
+                }
+            }
+        }
 
-        if self.secure {
+        if let (Some(ref path), &Some(ref cookie_path)) = (url.serialize_path(), &self.cookie.path) {
-            result = result && url.scheme == "https".to_string()
+            if !Cookie::path_match(path.as_slice(), cookie_path.as_slice()) {
+                return false;
             }
-        if self.http_only {
-            result = result && url.scheme.as_slice().starts_with("http")
         }
-        result
+
+        if self.cookie.secure && url.scheme != "https".to_string() {
+            return false;
+        }
+        if self.cookie.httponly && !url.scheme.as_slice().starts_with("http") {
+            return false;
+        }
+
+        return true;
     }
 }
 
@@ -300,9 +229,13 @@ impl CookieManager {
     }
 
     pub fn add(&mut self, cookie: &Cookie) -> bool {
-        match self.cookies.iter().find(|x| x.domain == cookie.domain && x.name == cookie.name && x.path == cookie.path) {
+        match self.cookies.iter().find(|x| {
+            x.cookie.domain == cookie.cookie.domain &&
+            x.cookie.name == cookie.cookie.name &&
+            x.cookie.path == cookie.cookie.path
+        }) {
             Some(c) => {
-                if c.http_only && !cookie.scheme.as_slice().starts_with("http") {
+                if c.cookie.httponly && !cookie.scheme.as_slice().starts_with("http") {
                     return false
                 }
             }

components/net/cookie_storage.rs

@@ -1,3 +1,10 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+//! Implementation of cookie storage as specified in
+//! http://tools.ietf.org/html/rfc6265
+
 use url::Url;
 use cookie::Cookie;
 
@@ -12,32 +19,64 @@ impl CookieStorage {
         }
     }
 
-    pub fn push(&mut self, cookie: Cookie) {
+    // http://tools.ietf.org/html/rfc6265#section-5.3
-        match self.cookies.iter().position(|c| c.domain == cookie.domain && c.path == cookie.path && c.name == cookie.name) {
+    pub fn remove(&mut self, cookie: &Cookie) -> Option<Cookie> {
-            Some(ind) => { self.cookies.remove(ind); }
+        // Step 1
-            None => {}
+        let position = self.cookies.iter().position(|c| {
-        };
+            c.cookie.domain == cookie.cookie.domain &&
+            c.cookie.path == cookie.cookie.path &&
+            c.cookie.name == cookie.cookie.name
+        });
 
-        self.cookies.push(cookie);
+        if let Some(ind) = position {
+            Some(self.cookies.remove(ind))
+        } else {
+            None
+        }
     }
 
+    // http://tools.ietf.org/html/rfc6265#section-5.3
+    pub fn push(&mut self, mut cookie: Cookie, request: &Url) {
+        // Step 11
+        if let Some(old_cookie) = self.remove(&cookie) {
+            // Step 11.2
+            if old_cookie.cookie.httponly && !request.scheme.starts_with("http") {
+                self.cookies.push(old_cookie);
+            } else {
+                // Step 11.3
+                cookie.created_at = old_cookie.created_at;
+                // Step 12
+                self.cookies.push(cookie);
+            }
+        }
+    }
+
+    // http://tools.ietf.org/html/rfc6265#section-5.4
     pub fn cookies_for_url(&mut self, url: Url) -> Option<String> {
         let filterer = |&:c: &&mut Cookie| -> bool {
-            error!(" === SENT COOKIE : {} {} {} {}", c.name, c.value, c.domain, c.path);
+            info!(" === SENT COOKIE : {} {} {:?} {:?}", c.cookie.name, c.cookie.value, c.cookie.domain, c.cookie.path);
-            error!(" === SENT COOKIE RESULT {}", c.appropriate_for_url(url.clone()));
+            info!(" === SENT COOKIE RESULT {}", c.appropriate_for_url(url.clone()));
+            // Step 1
             c.appropriate_for_url(url.clone())
         };
+
         let mut url_cookies = self.cookies.iter_mut().filter(filterer);
+
+        // TODO Step 2
+
         let reducer = |&:acc: String, c: &mut Cookie| -> String {
+            // Step 3
             c.touch();
+
+            // Step 4
             (match acc.len() {
                 0 => acc,
                 _ => acc + ";"
-            }) + c.name.as_slice() + "=" + c.value.as_slice()
+            }) + c.cookie.name.as_slice() + "=" + c.cookie.value.as_slice()
         };
         let result = url_cookies.fold("".to_string(), reducer);
 
-        error!(" === COOKIES SENT: {}", result);
+        info!(" === COOKIES SENT: {}", result);
         match result.len() {
             0 => None,
             _ => Some(result)

components/net/data_loader.rs

@@ -3,7 +3,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use resource_task::{Metadata, LoadData, TargetedLoadResponse, start_sending, ResponseSenders};
-use resource_task::ControlMsg;
 use resource_task::ProgressMsg::{Payload, Done};
 
 use serialize::base64::FromBase64;
@@ -13,15 +12,15 @@ use url::{percent_decode, SchemeData};
 
 use std::sync::mpsc::Sender;
 
-pub fn factory(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+pub fn factory(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>) {
     // NB: we don't spawn a new task.
     // Hypothesis: data URLs are too small for parallel base64 etc. to be worth it.
     // Should be tested at some point.
     // Left in separate function to allow easy moving to a task, if desired.
-    load(load_data, start_chan, cookies_chan)
+    load(load_data, start_chan)
 }
 
-fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>) {
     let url = load_data.url;
     assert!("data" == url.scheme.as_slice());
 
@@ -46,7 +45,7 @@ fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_c
     }
     let parts: Vec<&str> = scheme_data.as_slice().splitn(1, ',').collect();
     if parts.len() != 2 {
-        start_sending(senders, metadata, cookies_chan).send(Done(Err("invalid data uri".to_string()))).unwrap();
+        start_sending(senders, metadata).send(Done(Err("invalid data uri".to_string()))).unwrap();
         return;
     }
 
@@ -64,7 +63,7 @@ fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_c
     let content_type: Option<Mime> = ct_str.parse();
     metadata.set_content_type(content_type.as_ref());
 
-    let progress_chan = start_sending(senders, metadata, cookies_chan);
+    let progress_chan = start_sending(senders, metadata);
     let bytes = percent_decode(parts[1].as_bytes());
 
     if is_base64 {
@@ -96,9 +95,8 @@ fn assert_parse(url:          &'static str,
     use sniffer_task;
 
     let (start_chan, start_port) = channel();
-    let (cookies_chan, _) = channel();
     let sniffer_task = sniffer_task::new_sniffer_task();
-    load(LoadData::new(Url::parse(url).unwrap(), start_chan), sniffer_task, cookies_chan);
+    load(LoadData::new(Url::parse(url).unwrap(), start_chan), sniffer_task);
 
     let response = start_port.recv().unwrap();
     assert_eq!(&response.metadata.content_type, &content_type);

components/net/file_loader.rs

@@ -3,7 +3,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use resource_task::{ProgressMsg, Metadata, LoadData, start_sending, TargetedLoadResponse, ResponseSenders};
-use resource_task::ControlMsg;
 use resource_task::ProgressMsg::{Payload, Done};
 
 use std::borrow::ToOwned;
@@ -33,14 +32,14 @@ fn read_all(reader: &mut io::Stream, progress_chan: &Sender<ProgressMsg>)
     }
 }
 
-pub fn factory(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+pub fn factory(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>) {
     let url = load_data.url;
     assert!("file" == url.scheme.as_slice());
     let senders = ResponseSenders {
         immediate_consumer: start_chan,
         eventual_consumer: load_data.consumer,
     };
-    let progress_chan = start_sending(senders, Metadata::default(url.clone()), cookies_chan);
+    let progress_chan = start_sending(senders, Metadata::default(url.clone()));
     spawn_named("file_loader".to_owned(), move || {
         let file_path: Result<Path, ()> = url.to_file_path();
         match file_path {

components/net/http_loader.rs

@@ -2,7 +2,6 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use cookie::Cookie;
 use resource_task::{Metadata, TargetedLoadResponse, LoadData, start_sending_opt, ResponseSenders};
 use resource_task::ControlMsg;
 use resource_task::ProgressMsg::{Payload, Done};
@@ -11,36 +10,40 @@ use log;
 use std::collections::HashSet;
 use file_loader;
 use hyper::client::Request;
-use hyper::header::common::{ContentLength, ContentType, Host, Location};
+use hyper::header::common::{ContentLength, ContentType, Host, Location, SetCookie};
 use hyper::HttpError;
 use hyper::method::Method;
 use hyper::net::HttpConnector;
-use hyper::status::StatusClass;
+use hyper::status::{StatusCode, StatusClass};
 use std::error::Error;
 use openssl::ssl::{SslContext, SslVerifyMode};
 use std::io::{IoError, IoErrorKind, Reader};
-use std::sync::mpsc::Sender;
+use std::sync::mpsc::{Sender, channel};
+use std::thunk::Invoke;
 use util::task::spawn_named;
 use util::resource_files::resources_dir_path;
 use url::{Url, UrlParser};
 
 use std::borrow::ToOwned;
 
-pub fn factory(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+pub fn factory(cookies_chan: Sender<ControlMsg>)
+               -> Box<Invoke<(LoadData, Sender<TargetedLoadResponse>)> + Send> {
+    box move |:(load_data, start_chan)| {
         spawn_named("http_loader".to_owned(), move || load(load_data, start_chan, cookies_chan))
+    }
 }
 
-fn send_error(url: Url, err: String, senders: ResponseSenders, cookies_chan: Sender<ControlMsg>) {
+fn send_error(url: Url, err: String, senders: ResponseSenders) {
     let mut metadata = Metadata::default(url);
     metadata.status = None;
 
-    match start_sending_opt(senders, metadata, cookies_chan) {
+    match start_sending_opt(senders, metadata) {
         Ok(p) => p.send(Done(Err(err))).unwrap(),
         _ => {}
     };
 }
 
-fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
+fn load(mut load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_chan: Sender<ControlMsg>) {
     // FIXME: At the time of writing this FIXME, servo didn't have any central
     //        location for configuration. If you're reading this and such a
     //        repository DOES exist, please update this constant to use it.
@@ -59,22 +62,15 @@ fn load(load_data: LoadData, start_chan: Sender<TargetedLoadResponse>, cookies_c
         iters = iters + 1;
 
         if iters > max_redirects {
-            send_error(url, "too many redirects".to_string(), senders, cookies_chan);
+            send_error(url, "too many redirects".to_string(), senders);
             return;
         }
 
-        if redirected_to.contains(&url) {
-            send_error(url, "redirect loop".to_string(), senders, cookies_chan);
-            return;
-        }
-
-        redirected_to.insert(url.clone());
-
         match url.scheme.as_slice() {
             "http" | "https" => {}
             _ => {
                 let s = format!("{} request, but we don't support that scheme", url.scheme);
-                send_error(url, s, senders, cookies_chan);
+                send_error(url, s, senders);
                 return;
             }
         }
@@ -106,40 +102,63 @@ reason: \"certificate verify failed\" }]";
             },
             Err(e) => {
                 println!("{:?}", e);
-                send_error(url, e.description().to_string(), senders, cookies_chan);
+                send_error(url, e.description().to_string(), senders);
                 return;
             }
         };
 
+        let (tx, rx) = channel();
+        cookies_chan.send(ControlMsg::GetCookiesForUrl(url.clone(), tx));
+        if let Some(cookies) = rx.recv().unwrap() {
+            let mut v = Vec::new();
+            v.push(cookies.into_bytes());
+            load_data.headers.set_raw("Cookie".to_owned(), v);
+        }
+
+        // Avoid automatically preserving request headers when redirects occur.
+        // See https://bugzilla.mozilla.org/show_bug.cgi?id=401564 and
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=216828
+        if iters == 1 {
             // Preserve the `host` header set automatically by Request.
             let host = req.headers().get::<Host>().unwrap().clone();
             *req.headers_mut() = load_data.headers.clone();
             req.headers_mut().set(host);
+        }
+
         // FIXME(seanmonstar): use AcceptEncoding from Hyper once available
         //if !req.headers.has::<AcceptEncoding>() {
             // We currently don't support HTTP Compression (FIXME #2587)
             req.headers_mut().set_raw("Accept-Encoding".to_owned(), vec![b"identity".to_vec()]);
         //}
+        if log_enabled!(log::INFO) {
+            info!("{}", load_data.method);
+            for header in req.headers().iter() {
+                info!(" - {}", header);
+            }
+            info!("{:?}", load_data.data);
+        }
+
+        // Avoid automatically sending request body if a redirect has occurred.
         let writer = match load_data.data {
-            Some(ref data) => {
+            Some(ref data) if iters == 1 => {
                 req.headers_mut().set(ContentLength(data.len() as u64));
                 let mut writer = match req.start() {
                     Ok(w) => w,
                     Err(e) => {
-                        send_error(url, e.description().to_string(), senders, cookies_chan);
+                        send_error(url, e.description().to_string(), senders);
                         return;
                     }
                 };
                 match writer.write(data.as_slice()) {
                     Err(e) => {
-                        send_error(url, e.desc.to_string(), senders, cookies_chan);
+                        send_error(url, e.desc.to_string(), senders);
                         return;
                     }
                     _ => {}
                 };
                 writer
             },
-            None => {
+            _ => {
                 match load_data.method {
                     Method::Get | Method::Head => (),
                     _ => req.headers_mut().set(ContentLength(0))
@@ -147,7 +166,7 @@ reason: \"certificate verify failed\" }]";
                 match req.start() {
                     Ok(w) => w,
                     Err(e) => {
-                        send_error(url, e.description().to_string(), senders, cookies_chan);
+                        send_error(url, e.description().to_string(), senders);
                         return;
                     }
                 }
@@ -156,7 +175,7 @@ reason: \"certificate verify failed\" }]";
         let mut response = match writer.send() {
             Ok(r) => r,
             Err(e) => {
-                send_error(url, e.description().to_string(), senders, cookies_chan);
+                send_error(url, e.description().to_string(), senders);
                 return;
             }
         };
@@ -169,14 +188,8 @@ reason: \"certificate verify failed\" }]";
             }
         }
 
-        let mut cookies = Vec::new();
+        if let Some(&SetCookie(ref cookies)) = response.headers.get::<SetCookie>() {
-        for header in response.headers.iter() {
+            cookies_chan.send(ControlMsg::SetCookies(cookies.clone(), url.clone()));
-            if header.name().as_slice() == "Set-Cookie" {
-                match Cookie::new(header.value_string(), &url) {
-                    Some(cookie) => cookies.push(cookie),
-                    None => continue
-                }
-            }
         }
 
         if response.status.class() == StatusClass::Redirection {
@@ -187,7 +200,7 @@ reason: \"certificate verify failed\" }]";
                         Some(ref c) => {
                             if c.preflight {
                                 // The preflight lied
-                                send_error(url, "Preflight fetch inconsistent with main fetch".to_string(), senders, cookies_chan);
+                                send_error(url, "Preflight fetch inconsistent with main fetch".to_string(), senders);
                                 return;
                             } else {
                                 // XXXManishearth There are some CORS-related steps here,
@@ -199,12 +212,25 @@ reason: \"certificate verify failed\" }]";
                     let new_url = match UrlParser::new().base_url(&url).parse(new_url.as_slice()) {
                         Ok(u) => u,
                         Err(e) => {
-                            send_error(url, e.to_string(), senders, cookies_chan);
+                            send_error(url, e.to_string(), senders);
                             return;
                         }
                     };
                     info!("redirecting to {}", new_url);
                     url = new_url;
+
+                    if load_data.method == Method::Post &&
+                        (response.status == StatusCode::MovedPermanently ||
+                         response.status == StatusCode::Found) {
+                        load_data.method = Method::Get;
+                    }
+
+                    if redirected_to.contains(&url) {
+                        send_error(url, "redirect loop".to_string(), senders);
+                        return;
+                    }
+
+                    redirected_to.insert(url.clone());
                     continue;
                 }
                 None => ()
@@ -218,9 +244,8 @@ reason: \"certificate verify failed\" }]";
         });
         metadata.headers = Some(response.headers.clone());
         metadata.status = Some(response.status_raw().clone());
-        metadata.cookies = cookies;
 
-        let progress_chan = match start_sending_opt(senders, metadata, cookies_chan) {
+        let progress_chan = match start_sending_opt(senders, metadata) {
             Ok(p) => p,
             _ => return
         };

components/net/lib.rs

@@ -11,6 +11,7 @@
 #![allow(missing_copy_implementations)]
 #![allow(unstable)]
 
+extern crate "cookie" as cookie_rs;
 extern crate collections;
 extern crate geom;
 extern crate hyper;

components/net/resource_task.rs

@@ -10,8 +10,9 @@ use file_loader;
 use http_loader;
 use sniffer_task;
 use sniffer_task::SnifferTask;
-use cookie::Cookie;
+use cookie_rs::Cookie;
 use cookie_storage::CookieStorage;
+use cookie;
 
 use util::task::spawn_named;
 
@@ -24,11 +25,15 @@ use url::Url;
 
 use std::borrow::ToOwned;
 use std::sync::mpsc::{channel, Receiver, Sender};
+use std::thunk::Invoke;
 
 pub enum ControlMsg {
     /// Request the data associated with a particular URL
     Load(LoadData),
-    Cookies(Vec<Cookie>),
+    /// Store a set of cookies for a given originating URL
+    SetCookies(Vec<Cookie>, Url),
+    /// Retrieve the stored cookies for a given URL
+    GetCookiesForUrl(Url, Sender<Option<String>>),
     Exit
 }
 
@@ -80,8 +85,6 @@ pub struct Metadata {
 
     /// HTTP Status
     pub status: Option<RawStatus>,
-
-    pub cookies: Vec<Cookie>
 }
 
 impl Metadata {
@@ -94,7 +97,6 @@ impl Metadata {
             headers: None,
             // http://fetch.spec.whatwg.org/#concept-response-status-message
             status: Some(RawStatus(200, "OK".to_owned())),
-            cookies: Vec::new(),
         }
     }
 
@@ -147,14 +149,13 @@ pub enum ProgressMsg {
 }
 
 /// For use by loaders in responding to a Load message.
-pub fn start_sending(senders: ResponseSenders, metadata: Metadata, cookies_chan: Sender<ControlMsg>) -> Sender<ProgressMsg> {
+pub fn start_sending(senders: ResponseSenders, metadata: Metadata) -> Sender<ProgressMsg> {
-    start_sending_opt(senders, metadata, cookies_chan).ok().unwrap()
+    start_sending_opt(senders, metadata).ok().unwrap()
 }
 
 /// For use by loaders in responding to a Load message.
-pub fn start_sending_opt(senders: ResponseSenders, metadata: Metadata, cookies_chan: Sender<ControlMsg>) -> Result<Sender<ProgressMsg>, ()> {
+pub fn start_sending_opt(senders: ResponseSenders, metadata: Metadata) -> Result<Sender<ProgressMsg>, ()> {
     let (progress_chan, progress_port) = channel();
-    let cookies = metadata.cookies.clone();
     let result = senders.immediate_consumer.send(TargetedLoadResponse {
         load_response: LoadResponse {
             metadata:      metadata,
@@ -162,10 +163,6 @@ pub fn start_sending_opt(senders: ResponseSenders, metadata: Metadata, cookies_c
         },
         consumer: senders.eventual_consumer
     });
-    match cookies_chan.send(ControlMsg::Cookies(cookies)) {
-        Ok(_) => {}
-        Err(_) => { return Err(()) }
-    }
     match result {
         Ok(_) => Ok(progress_chan),
         Err(_) => Err(())
@@ -231,11 +228,20 @@ impl ResourceManager {
               ControlMsg::Load(load_data) => {
                 self.load(load_data)
               }
-              ControlMsg::Cookies(vector) => {
+              ControlMsg::SetCookies(vector, request) => {
-                for cookie in vector.iter() {
+                for cookie in vector.into_iter() {
-                    self.cookie_storage.push(cookie.clone());
+                    if let Some(cookie) = cookie::Cookie::new_wrapped(cookie, &request) {
+                        if cookie.cookie.value.is_empty() {
+                            self.cookie_storage.remove(&cookie);
+                        } else {
+                            self.cookie_storage.push(cookie, &request);
                         }
                     }
+                }
+              }
+              ControlMsg::GetCookiesForUrl(url, consumer) => {
+                consumer.send(self.cookie_storage.cookies_for_url(url));
+              }
               ControlMsg::Exit => {
                 break
               }
@@ -246,29 +252,33 @@ impl ResourceManager {
     fn load(&mut self, load_data: LoadData) {
         let mut load_data = load_data;
         self.user_agent.as_ref().map(|ua| load_data.headers.set(UserAgent(ua.clone())));
-        if let Some(cookies) = self.cookie_storage.cookies_for_url(load_data.url.clone()) {
-            let mut v = Vec::new();
-            v.push(cookies.into_bytes());
-            load_data.headers.set_raw("cookie".to_owned(), v);
-        }
         let senders = ResponseSenders {
             immediate_consumer: self.sniffer_task.clone(),
             eventual_consumer: load_data.consumer.clone(),
         };
 
-        debug!("resource_task: loading url: {}", load_data.url.serialize());
+        fn from_factory(factory: fn(LoadData, Sender<TargetedLoadResponse>))
-        match load_data.url.scheme.as_slice() {
+                        -> Box<Invoke<(LoadData, Sender<TargetedLoadResponse>)> + Send> {
-            "file" => file_loader::factory(load_data, self.sniffer_task.clone(), self.cookies_chan.clone()),
+            box move |&:(load_data, start_chan)| {
-            "http" | "https" => http_loader::factory(load_data, self.sniffer_task.clone(), self.cookies_chan.clone()),
+                factory(load_data, start_chan)
-            "data" => data_loader::factory(load_data, self.sniffer_task.clone(), self.cookies_chan.clone()),
+            }
-            "about" => about_loader::factory(load_data, self.sniffer_task.clone(), self.cookies_chan.clone()),
+        }
+
+        let loader = match load_data.url.scheme.as_slice() {
+            "file" => from_factory(file_loader::factory),
+            "http" | "https" => http_loader::factory(self.cookies_chan.clone()),
+            "data" => from_factory(data_loader::factory),
+            "about" => from_factory(about_loader::factory),
             _ => {
                 debug!("resource_task: no loader for scheme {}", load_data.url.scheme);
-                start_sending(senders, Metadata::default(load_data.url), self.cookie_chan.clone())
+                start_sending(senders, Metadata::default(load_data.url))
                     .send(ProgressMsg::Done(Err("no loader for scheme".to_string()))).unwrap();
                 return
             }
-        }
+        };
+        debug!("resource_task: loading url: {}", load_data.url.serialize());
+
+        loader.invoke((load_data, self.sniffer_task.clone()));
     }
 }
 

components/servo/Cargo.lock

@@ -559,6 +559,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 name = "net"
 version = "0.0.1"
 dependencies = [
+ "cookie 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "geom 0.1.0 (git+https://github.com/servo/rust-geom)",
  "hyper 0.1.1 (git+https://github.com/servo/hyper?branch=servo)",
  "openssl 0.2.15 (registry+https://github.com/rust-lang/crates.io-index)",