bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl #15775

avadacatavra · 2017-03-01T01:03:55Z

fixed new wrapped hyper to handle strings per hyper 10

bumped to cookie 0.6

use serde_json to persist cookies in net crate

fixed tests to be compatible with new cookie version

fixed case mismatch with domain test

bumped to webdriver 0.22

Bumped hyper, cookie and webdriver. Added a dependency on hyper_openssl as part of the bump.

It's important to point out that I had to fork and update websocket. I spoke with @nox about websocket/ws. This branch points to my fork. Until we migrate to ws (#14517), we'll need to maintain a fork.

Also, I believe this may fix #10550, but that might require additional verification

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14203 and #15010 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-03-01T01:04:00Z

Heads up! This PR modifies the following files:

@asajeffrey: components/webdriver_server/lib.rs, components/webdriver_server/Cargo.toml
@jgraham: components/webdriver_server/lib.rs, components/webdriver_server/Cargo.toml
@fitzgen: components/devtools_traits/Cargo.toml, components/devtools_traits/Cargo.toml, components/devtools/Cargo.toml, components/script/dom/document.rs, components/script/webdriver_handlers.rs, components/script/Cargo.toml, components/script/dom/websocket.rs, components/script_traits/webdriver_msg.rs, components/script_traits/webdriver_msg.rs, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml
@KiChjang: components/net/lib.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/net/connector.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/webdriver_handlers.rs, components/script/Cargo.toml, components/net/cookie_storage.rs, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml, components/net/resource_thread.rs, components/net/Cargo.toml, components/net/cookie.rs, components/net/subresource_integrity.rs, components/script/dom/websocket.rs, components/script_traits/webdriver_msg.rs, components/script_traits/webdriver_msg.rs, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml

KiChjang · 2017-03-01T01:15:00Z

This requires a rebase, unfortunately.

nox · 2017-03-01T08:44:41Z

components/net/Cargo.toml

@@ -12,11 +12,12 @@ path = "lib.rs"
 [dependencies]
 brotli = "1.0.6"
 content-blocker = "0.2.3"
-cookie = "0.2.5"
+cookie = {version = "0.6", features = ["rustc-serialize"]}


No need for that feature anymore.

nox

Did a first review pass, most comments are superficial, great work. 🍷

nox · 2017-03-01T09:12:13Z

components/net/connector.rs

+        .expect("Need certificate file to make network requests")
+        .join(certificate_file);
+
+    let mut ssl_connector_builder = SslConnectorBuilder::new(SslMethod::tls()).unwrap();


SslMethod::tls() replaces SslMethod::Sslv23 right?

nox · 2017-03-01T09:12:13Z

components/net/cookie.rs

@@ -34,13 +34,23 @@ pub struct Cookie {
 }

 impl Cookie {
+    pub fn new_wrapped_hyper(cookie_str: String, request: &ServoUrl,


Rename to from_cookie_string.

nox · 2017-03-01T09:12:13Z

components/net/cookie.rs

+        match cookie {
+            Ok(cookie) => Cookie::new_wrapped(cookie, request, source),
+            Err(_) => None,
+        }


I'm pretty sure you don't need the type annotation and whatnot here.

cookie_rs::Cookie::parse(cookie_str) .ok() .map(|cookie| Cookie::new_wrapped(cookie, request, source))

nox · 2017-03-01T09:12:13Z

components/net/cookie.rs

                       -> Option<Cookie> {
+        let mut cookie = cookie.into_owned();


This is unneeded given cookie is already a Cookie<'static>.

nox · 2017-03-01T09:12:13Z

components/net/cookie.rs

        // Step 3
-        let (persistent, expiry_time) = match (&cookie.max_age, &cookie.expires) {
+        let (persistent, expiry_time) = match (&cookie.max_age(), &cookie.expires()) {


No need for the & anymore to appease the borrow checker, could you clean it up a bit?

nox · 2017-03-01T09:12:13Z

components/script/webdriver_handlers.rs

        HTTP
    } else {
        NonHTTP
    };
-    reply.send(match (document.is_cookie_averse(), cookie.domain.clone()) {
+    let cookie2 = cookie.clone().to_owned();


Why clone cookie?

nox · 2017-03-01T09:12:13Z

components/script/webdriver_handlers.rs

        HTTP
    } else {
        NonHTTP
    };
-    reply.send(match (document.is_cookie_averse(), cookie.domain.clone()) {
+    let cookie2 = cookie.clone().to_owned();
+    let domain = cookie2.domain().clone().to_owned();


Why clone cookie's domain?

nox · 2017-03-01T09:12:13Z

components/webdriver_server/Cargo.toml

@@ -10,9 +10,9 @@ name = "webdriver_server"
 path = "lib.rs"

 [dependencies]
-cookie = "0.2.5"
+cookie = {version = "0.6", features = ["rustc-serialize"]}


This feature is useless now.

nox · 2017-03-01T09:12:13Z

components/webdriver_server/lib.rs


    /// Time to wait for a page to finish loading upon navigation.
-    load_timeout: u32,
+    load_timeout: Option<u64>,


Why an Option now?

i changed it to an option to be consistent with the TimeoutParameters in webdriver

nox · 2017-03-01T09:12:13Z

components/webdriver_server/lib.rs

@@ -681,7 +699,7 @@ impl Handler {
            secure: params.secure,
            httponly: params.httpOnly,
            custom: BTreeMap::new()
-        };
+        };*/


Please remove the commented out code.

avadacatavra · 2017-03-01T17:24:33Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/cookie.rs, line 37 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Rename to from_cookie_string.

Done.

components/net/cookie.rs, line 43 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

I'm pretty sure you don't need the type annotation and whatnot here.
cookie_rs::Cookie::parse(cookie_str)
    .ok()
    .map(|cookie| Cookie::new_wrapped(cookie, request, source))

Done.

components/net/cookie.rs, line 49 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

This is unneeded given cookie is already a Cookie<'static>.

Done.

components/net/cookie.rs, line 51 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

No need for the & anymore to appease the borrow checker, could you clean it up a bit?

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:25:15Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/Cargo.toml, line 15 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

No need for that feature anymore.

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:27:07Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/http_loader.rs, line 169 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Seems like an error, what's up with all that commented-out code?

Done.

components/net/http_loader.rs, line 202 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

I think it would be better if we stored the proper type of error that we get instead of String, but that can be filed as a follow-up.

let's do that as a follow up so we can get this landed

components/net/http_loader.rs, line 1137 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Indentation is broken.

Done.

components/net/http_loader.rs, line 1146 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Indentation is broken.

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:27:27Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/lib.rs, line 46 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Nit: spurious newline.

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:27:36Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/resource_thread.rs, line 336 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Indentation is broken.

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:28:40Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/connector.rs, line 36 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

yup!

Done.

Comments from Reviewable

avadacatavra · 2017-03-01T17:30:35Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net/http_loader.rs, line 205 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Shouldn't this be implemented on LoadError?

can we make this a follow up

components/net/http_loader.rs, line 211 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Shouldn't this be implemented on LoadError?

can we make this a followup?

Comments from Reviewable

avadacatavra · 2017-03-01T17:36:11Z

Review status: 0 of 27 files reviewed at latest revision, 32 unresolved discussions.

components/net_traits/Cargo.toml, line 13 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

This feature is useless now.

Done.

components/net_traits/lib.rs, line 383 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Indentation is broken. Please realign as it was before.

Done.

components/net_traits/lib.rs, line 398 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Comment is broken.

Done.

components/script/Cargo.toml, line 36 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

This feature is useless now.

Done.

components/script/webdriver_handlers.rs, line 231 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Why? cookie is already a Cookie<'static>.

Done.

components/script/dom/document.rs, line 3106 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Oh I see now why things have been renamed. What about renaming SetCookieForUrl to SetCookieHeaderForUrl and keep SetCookiesForUrl as is?

Done.

components/script/dom/websocket.rs, line 504 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

This can be written in a shorter way:
cookies.iter().map(ToString::to_string).collect::<String>()

Done.

components/webdriver_server/Cargo.toml, line 13 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

This feature is useless now.

Done.

components/webdriver_server/lib.rs, line 702 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Please remove the commented out code.

Done.

Comments from Reviewable

nox · 2017-03-01T17:47:53Z

Reviewed 10 of 27 files at r1, 17 of 17 files at r2.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

components/net/http_loader.rs, line 202 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

let's do that as a follow up so we can get this landed

Ok, don't forget to file it.

components/net/http_loader.rs, line 205 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

can we make this a follow up

Ok, don't forget to file it.

components/net/http_loader.rs, line 211 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

can we make this a followup?

Ok, don't forget to file it.

components/net_traits/lib.rs, line 398 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

Done.

You sure?

components/webdriver_server/lib.rs, line 112 at r1 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

i changed it to an option to be consistent with the TimeoutParameters in webdriver

Oh, nice catch.

Comments from Reviewable

jdm · 2017-03-03T06:45:06Z

FYI: I haven't been following any of the discussion due to travel. Use your best judgement.

nox · 2017-03-03T09:28:42Z

@jdm We will just lowercase all the things and file a followup then.

-S-awaiting-review +S-needs-code-changes

Reviewed 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/net/resource_thread.rs, line 329 at r8 (raw file):

            }
        }
    }*/

Just remove the method.

components/net_traits/lib.rs, line 377 at r8 (raw file):

    /// Try to make a websocket connection to a URL.
    WebsocketConnect(WebSocketCommunicate, WebSocketConnectData),
    /// Store a set of cookies from a raw cookie header

I'm afraid my friend that this comment suggestion was for when SetCookieForUrl was holding a String. :P

The good news though is that you can just use the comment that was here before your changes.

components/script/dom/document.rs, line 3113 at r8 (raw file):

                          },
            Err(_) => return Err(Error::Security)
        }

A few final changes needed:

This should first parse the input (cookie) as a SetCookie header.
This should use SetCookiesForUrl.
This should not fail on parsing errors.

if let Ok(cookie_header) = SetCookie::parse_header(&[cookie.into_bytes()]) {
    let cookies = cookie_header.1.into_iter().filter_map(|cookie| {
        cookie_rs::Cookie::parse(cookie).ok().map(Serde)
    });
    let _ = self.window....;
}

Comments from Reviewable

avadacatavra · 2017-03-06T00:32:26Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

components/net/resource_thread.rs, line 168 at r6 (raw file):

Previously, nox (Anthony Ramine) wrote…

This used to be a loop over cookies with a call to set_cookie_for_url, please go back to that version.

Done.

components/net/resource_thread.rs, line 329 at r8 (raw file):

Previously, nox (Anthony Ramine) wrote…

Just remove the method.

sorry forgot to delete that :) done

components/net_traits/lib.rs, line 377 at r8 (raw file):

Previously, nox (Anthony Ramine) wrote…

I'm afraid my friend that this comment suggestion was for when SetCookieForUrl was holding a String. :P

The good news though is that you can just use the comment that was here before your changes.

Done.

components/script/dom/document.rs, line 3106 at r6 (raw file):

Previously, nox (Anthony Ramine) wrote…

So I've come to realise that we shouldn't have touched SetCookieForUrl at all, and that this code here should use SetCookiesForUrl, parsing the header and then each part as a cookie like in websocket.rs.

This way we still have only two commands SetCookieForUrl and SetCookiesForUrl just like before, and we can avoid the silly vec![] expression in webdriver_handlers.rs.

Please do it now or file a follow-up about it.

Done.

components/script/dom/document.rs, line 3113 at r8 (raw file):

Previously, nox (Anthony Ramine) wrote…

A few final changes needed:

This should first parse the input (cookie) as a SetCookie header.

This should use SetCookiesForUrl.

This should not fail on parsing errors.
if let Ok(cookie_header) = SetCookie::parse_header(&[cookie.into_bytes()]) {
    let cookies = cookie_header.1.into_iter().filter_map(|cookie| {
        cookie_rs::Cookie::parse(cookie).ok().map(Serde)
    });
    let _ = self.window....;
}

Done.

Comments from Reviewable

avadacatavra · 2017-03-06T13:37:18Z

Review status: 24 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/net/resource_thread.rs, line 329 at r8 (raw file):

Previously, avadacatavra (Diane Hosfelt) wrote…

sorry forgot to delete that :) done

Done.

Comments from Reviewable

nox · 2017-03-07T08:37:03Z

-S-awaiting-review -S-needs-rebase +S-needs-code-changes

Reviewed 8 of 8 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

a discussion (no related file):
You seem to have ran ./mach cargo-update -a, don't do that, and please revert the unrelated bumps from Cargo.lock.

components/net_traits/lib.rs, line 377 at r9 (raw file):

    /// Try to make a websocket connection to a URL.
    WebsocketConnect(WebSocketCommunicate, WebSocketConnectData),
    /// Store a set of cookies and set cookies for header for a given originating URL

That comment is still wrong?

components/script/dom/document.rs, line 3113 at r8 (raw file):
There should be no error case for failing to parse, check the code before all your changes.

As I said earlier:

This should not fail on parsing errors.

Remove the whole else branch thanks.

Comments from Reviewable


        updated files for openssl bump, changed to hyper_openssl::opensslclient

fixed new wrapped hyper to handle strings per hyper 10 bumped to cookie 0.6 use serde_json to persist cookies in net crate fixed tests to be compatible with new cookie version fixed case mismatch with domain test bumped to webdriver 0.22

nox · 2017-03-08T08:15:15Z

-S-awaiting-review +S-needs-code-changes

Reviewed 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/script/Cargo.toml, line 61 at r10 (raw file):

mime_guess = "1.8.0"
msg = {path = "../msg"}
net = {path = "../net"}

This is unneeded.

Comments from Reviewable

bors-servo · 2017-03-08T10:52:20Z

☔ The latest upstream changes (presumably #15849) made this pull request unmergeable. Please resolve the merge conflicts.

nox · 2017-03-08T11:04:05Z

Fixed that last nit myself and rebased it in #15868. Congrats @avadacatavra.

highfive assigned pcwalton Mar 1, 2017

highfive added the S-awaiting-review label Mar 1, 2017

KiChjang added S-needs-rebase and removed S-awaiting-review labels Mar 1, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch from 1245613 to 43bb670 Mar 1, 2017

highfive added the S-awaiting-review label Mar 1, 2017

avadacatavra changed the title ~~updated files for openssl bump, changed to hyper_openssl::opensslclient~~ bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl Mar 1, 2017

nox assigned nox and unassigned pcwalton Mar 1, 2017

nox reviewed Mar 1, 2017

View changes

nox requested changes Mar 1, 2017

View changes

nox added S-needs-code-changes and removed S-needs-rebase S-awaiting-review labels Mar 1, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch from 43bb670 to ddce34b Mar 1, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 1, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Mar 1, 2017

highfive added the S-needs-rebase label Mar 2, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch from 5160bcc to 66add34 Mar 3, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 3, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Mar 3, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch from 66add34 to bd4864c Mar 6, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 6, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch 2 times, most recently from 5128ad4 to fc0c616 Mar 6, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Mar 7, 2017

nox removed the S-needs-rebase label Mar 7, 2017

avadacatavra force-pushed the avadacatavra:bump-hyper branch from fc0c616 to c9df2b0 Mar 7, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 7, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Mar 8, 2017

highfive added the S-needs-rebase label Mar 8, 2017

nox closed this Mar 8, 2017

servo / servo

bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl #15775

bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl #15775

avadacatavra commented Mar 1, 2017 •

edited by SimonSapin

highfive commented Mar 1, 2017

KiChjang commented Mar 1, 2017

This comment has been minimized.

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

nox commented Mar 1, 2017

jdm commented Mar 3, 2017

nox commented Mar 3, 2017

avadacatavra commented Mar 6, 2017

avadacatavra commented Mar 6, 2017

nox commented Mar 7, 2017

nox commented Mar 8, 2017

bors-servo commented Mar 8, 2017

nox commented Mar 8, 2017

servo / servo

Join GitHub today

bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl #15775

bumped hyper to 0.10, cookie to 0.6, webdriver to 0.22, added dependency on hyper-openssl #15775

Conversation

avadacatavra commented Mar 1, 2017 • edited by SimonSapin

highfive commented Mar 1, 2017

KiChjang commented Mar 1, 2017

This comment has been minimized.

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

avadacatavra commented Mar 1, 2017

nox commented Mar 1, 2017

jdm commented Mar 3, 2017

nox commented Mar 3, 2017

avadacatavra commented Mar 6, 2017

avadacatavra commented Mar 6, 2017

nox commented Mar 7, 2017

nox commented Mar 8, 2017

bors-servo commented Mar 8, 2017

nox commented Mar 8, 2017

avadacatavra commented Mar 1, 2017 •

edited by SimonSapin