Allow Request's Headers to be created with various objects #13802

jeenalee · 2016-10-17T16:15:48Z

While Headers could be constructed correctly with an array or
object (open ended dictionary/MozMap), Request's Headers failed to be
created with non-Headers object (such as array or open ended
dictionary/MozMap).

Before, Request's Headers could be filled with only a Headers object in
Step 28. This has been expanded to accommodate array and open ended
dictionary.

Step 29 empties the Request's Headers list after it had been filled in
Step 28, thus resulting in an empty Headers object when it shouldn't
be. This step has been removed with a comment in this commit.

If a RequestInit Headers is not given, but a RequestInfo Headers is
given, RequestInfo Headers should be used to construct Request
Headers. That step has been added after Step 31.

Corresponding wpt result is updated in this commit.

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

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

This change is


        Allow Request's Headers to be created with various objects

While Headers could be constructed correctly with an array or object (open ended dictionary/MozMap), Request's Headers failed to be created with non-Headers object (such as array or open ended dictionary/MozMap). Before, Request's Headers could be filled with only a Headers object in Step 28. This has been expanded to accommodate array and open ended dictionary. Step 29 empties the Request's Headers list after it had been filled in Step 28, thus resulting in an empty Headers object when it shouldn't be. This step has been removed with a comment in this commit. If a RequestInit Headers is *not* given, but a RequestInfo Headers is given, RequestInfo Headers should be used to construct Request Headers. That step has been added after Step 31. Corresponding wpt result is updated in this commit.

highfive · 2016-10-17T16:15:52Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/request.rs
@KiChjang: components/script/dom/request.rs

emilio

Sorry for the huge delay @jeenalee :(. Feel free to ping me more often if you see this happens again.

emilio · 2016-10-23T23:30:33Z

components/script/dom/request.rs

            }
        }

        // Step 29
-        r.Headers().empty_header_list();
+        // When r.Headers()'s header list is emptied,
+        // r.Headers that was filled in Step 28 becomes empty.


This comment seems confusing to me, could you explain or rephrase it? It seems we can't empty these because otherwise we undo the stuff done above?

If that's the case, we should be doing a deep copy in step 27 then instead.

That's right! Sorry for the confusing wording. We can't empty them because we would undo the stuff done above. I tried your suggestion on doing a deep copy, and got the following:

I cannot understand why, but the following approach does not make the subtest [Testing request header creations with various objects] in request-headers.html pass.
Step 27: let mut headers_copy = r.Headers().clone();
Step 28: fill in headers_copy using HeadersInit (unchanged from the original pull request)
Step 29: r.Headers().empty_header_list();
Step 30: Check whether Request Mode is NoCors (unchanged from the original pull request)
Step 31: After addressing your suggestion below this comment:

match init.headers { None => { // This is equivalent to the specification's concept of // "associated headers list". If an init headers is not given, // but an input with headers is given, set request's // headers as the input's Headers. if let RequestInfo::Request(ref input_request) = input { try!(r.Headers().fill(Some(HeadersInit::Headers(input_request.Headers())))); } }, Some(_) => try!(r.Headers().fill(Some(HeadersInit::Headers(headers_copy)))), }

Because we set the headers_copy to be a deep copy of r.Headers(), then fill r.Headers() with the headers_copy in Step 31, I think the subtest shouldn't fail.

When we do a deep clone in Step 27, but not empty r.Headers().empty_header_list(); in Step 29, when running wpt, Servo crashes with a message that begins with DOMRefCell<T> already borrowed: BorrowMutError. Here's the backtrace. The error occurs in Header's fill method.

I'm confused! I feel like I'm missing something. Do you have any insights?

So that backtrace can come from anywhere in the constructor, so it's hard to diagnose based on that.

But a borrow error indicates that you're reading from something while writing to it in general. Something like filling r.headers() with themselves, or holding a borrow to somewhere inside the Headers object can be potential problems.

Hmm. I realized I wasn't making a deep copy. I had to do r.Headers.r().clone() in order to actually make a deep copy.

With this approach though, because r.Headers() is a Root<T>, it becomes difficult to make a mutable reference to r.Headers(). Without the mutable reference, we cannot set r.Headers() to be the deep copied headers in Step 27.

I want to suggest we leave the comment more descriptive (as you wrote above), but not make a deep copy in Step 27.

emilio · 2016-10-23T23:30:33Z

components/script/dom/request.rs

+        // "associated headers list". If an init headers is not given,
+        // but an input with headers is given, set request's
+        // headers as the input's Headers.
+        if let None = init.headers {


nit: if init.headers.is_none(), though this can probably be more legible with a match statement combined with the above if let.

emilio · 2016-10-23T23:30:33Z

components/script/dom/request.rs

-        if let RequestInfo::Request(ref input_request) = input {
-            headers_copy = input_request.Headers();
-        }
-
        // Step 28


The step numbers seem to have changed (though that's probably my fault!), feel free to update them if you feel like it :).

I'll leave them as it is for now! It's probably more straightforward to fix them when we update Request Constructor to match the spec :)

jeenalee · 2016-10-24T18:15:18Z

@emilio Thanks Emilio! I realized that the Fetch spec has been updated, but our request constructor has not yet. The changes in this PR were based on the older Fetch spec version. Will it make more sense to close this PR, and update the request constructor to match the current Fetch spec first?

jdm · 2016-10-24T18:31:37Z

Updating the numbers in a separate commit in the same PR is fine; or doing it separately after. I doubt it's necessary to close this one though.


        Address review comments

KiChjang · 2016-10-27T22:48:16Z

components/script/dom/request.rs

+        // `r.Headers()` is a `Root<T>`, and therefore it is difficult
+        // to obtain a mutable reference to `r.Headers()`. Without the
+        // mutable reference, we cannot mutate `r.Headers()` to be the
+        // deep copied headers in Step 27.


Does r.headers.set(headers_copy) not work? If so, then you can just create pub fn set_headers(&self, headers: Headers) in impl Request.

Good idea! Hmm, though while it compiles, more tests fail with that change. I'm not sure why.

Hmmm... So it seems to me we'd need to add a method to Request that creates a new Request with a clone of the inner hyper list, is that right?

I'm totally fine doing it as a followup if @jdm agrees.


        Remove unused use statement

jeenalee · 2016-10-28T04:56:28Z

I thought this would be a small change until the request Constructor gets updated to match the Fetch spec. While this patch isn't the perfect fix, I feel like we're starting to go down the rabbit hole... I'd say we move forward with the not-perfect fix, or I can close this pull request and focus on updating the constructor!

emilio

The rest looks ok to me, thanks for looking into this @jeenalee! :)

emilio · 2016-10-28T10:22:34Z

components/script/dom/request.rs

+        // `r.Headers()` is a `Root<T>`, and therefore it is difficult
+        // to obtain a mutable reference to `r.Headers()`. Without the
+        // mutable reference, we cannot mutate `r.Headers()` to be the
+        // deep copied headers in Step 27.


Hmmm... So it seems to me we'd need to add a method to Request that creates a new Request with a clone of the inner hyper list, is that right?

I'm totally fine doing it as a followup if @jdm agrees.

jeenalee · 2016-11-07T21:56:50Z

Hi! Just checking in with this PR. Please let me know if there's anything I can do :)

jdm · 2016-11-07T22:55:15Z

Whoops, I didn't realize this was waiting on my input. I'm not thrilled with either the before or after code (or the spec, for that matter), but I don't have concrete suggestions for how to improve any of those pieces. Let's merge this and move on!

@bors-servo: r=emilio

bors-servo · 2016-11-07T22:55:18Z

📌 Commit e821da8 has been approved by emilio

bors-servo · 2016-11-07T23:52:49Z

⌛ Testing commit e821da8 with merge 4fc283e...


        Auto merge of #13802 - jeenalee:request-headers, r=emilio

Allow Request's Headers to be created with various objects  While Headers could be constructed correctly with an array or object (open ended dictionary/MozMap), Request's Headers failed to be created with non-Headers object (such as array or open ended dictionary/MozMap). Before, Request's Headers could be filled with only a Headers object in Step 28. This has been expanded to accommodate array and open ended dictionary. Step 29 empties the Request's Headers list after it had been filled in Step 28, thus resulting in an empty Headers object when it shouldn't be. This step has been removed with a comment in this commit. If a RequestInit Headers is _not_ given, but a RequestInfo Headers is given, RequestInfo Headers should be used to construct Request Headers. That step has been added after Step 31. Corresponding wpt result is updated in this commit. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13758 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13802)

bors-servo · 2016-11-08T00:49:51Z

💔 Test failed - linux-rel-wpt

jdm · 2016-11-08T00:56:33Z

Those are some very unhappy tests! One clear problem:

  ▶ CRASH [expected OK] /fetch/api/request/request-consume.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ DOMRefCell<T> already borrowed: BorrowMutError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at ../src/libcore/result.rs:837)
  │ stack backtrace:
  │    0:     0x7fb63064eafd - backtrace::backtrace::trace::h16372ee7bf1517e5
  │    1:     0x7fb63064f192 - backtrace::capture::Backtrace::new::hcc43c50c4b11c693
  │    2:     0x7fb62f3b00fd - servo::main::_{{closure}}::h8e9c5971861a4720
  │    3:     0x7fb631015b24 - std::panicking::rust_panic_with_hook
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:452
  │    4:     0x7fb6310159e4 - std::panicking::begin_panic<collections::string::String>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:413
  │    5:     0x7fb631015909 - std::panicking::begin_panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:397
  │    6:     0x7fb631015897 - std::panicking::rust_begin_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:373
  │    7:     0x7fb6310522fd - core::panicking::panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libcore/panicking.rs:69
  │    8:     0x7fb62f9a5035 - core::result::unwrap_failed::h643f371f836203d8
  │    9:     0x7fb62fb348e6 - _<script..dom..headers..Headers as script..dom..bindings..codegen..Bindings..HeadersBinding..HeadersBinding..HeadersMethods>::Append::h1c59f2b40c66fb7e
  │   10:     0x7fb62fb356ce - script::dom::headers::Headers::fill::hd3324ebf20b36137
  │   11:     0x7fb62fbce43f - script::dom::request::Request::Constructor::h3329b257fb1e6789
  │   12:     0x7fb62f896371 - std::panicking::try::do_call::h6e04d3e7d58d6235

I suspect this is the case where headers_copy is both the object being used to fill the new request's headers and the headers being filled - we'll be calling borrow_mut on one and borrow on the other, which won't work.


        Fill r's headers with headers_copy when HeadersInit::Headers is given

Instead of filling request's headers whenever a `HeadersInit` is given, this patch fills request's headers only when `HeadersInit` with a type of `Headers` is given. Previously, the constructor tried to fill request's headers with itself, causing Servo to crash.

jeenalee · 2016-11-08T05:58:11Z

Oopsies, that's bad! The latest commit should fix that crash.

KiChjang · 2016-11-08T06:00:50Z

@bors-servo try

bors-servo · 2016-11-08T06:00:53Z

⌛ Trying commit bfd999f with merge 21bc4e0...


        Auto merge of #13802 - jeenalee:request-headers, r=<try>

Allow Request's Headers to be created with various objects  While Headers could be constructed correctly with an array or object (open ended dictionary/MozMap), Request's Headers failed to be created with non-Headers object (such as array or open ended dictionary/MozMap). Before, Request's Headers could be filled with only a Headers object in Step 28. This has been expanded to accommodate array and open ended dictionary. Step 29 empties the Request's Headers list after it had been filled in Step 28, thus resulting in an empty Headers object when it shouldn't be. This step has been removed with a comment in this commit. If a RequestInit Headers is _not_ given, but a RequestInfo Headers is given, RequestInfo Headers should be used to construct Request Headers. That step has been added after Step 31. Corresponding wpt result is updated in this commit. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13758 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13802)

bors-servo · 2016-11-08T06:57:35Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-08T06:57:38Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html aed6845267bba6c52d3aa69c4889449b454d8117
/_mozilla/css/inline_block_opacity_change_ref.html febec1b4730afab195f280694dad47200b09ea3c
Testing aed6845267bba6c52d3aa69c4889449b454d8117 == febec1b4730afab195f280694dad47200b09ea3c

emilio · 2016-11-09T16:07:23Z

@bors-servo r+

#13480

bors-servo · 2016-11-09T16:07:25Z

📌 Commit bfd999f has been approved by emilio

KiChjang · 2016-11-09T18:44:45Z

@bors-servo retry clean

jdm · 2016-11-09T19:13:28Z

@bors-servo: r-
Hold on, I just want to make sure I understand the implications of the latest code change.

jdm · 2016-11-09T19:28:55Z

@bors-servo: r+
Ok, I understand how it works.

bors-servo · 2016-11-09T19:28:56Z

📌 Commit bfd999f has been approved by jdm


        Auto merge of #13802 - jeenalee:request-headers, r=jdm

Allow Request's Headers to be created with various objects  While Headers could be constructed correctly with an array or object (open ended dictionary/MozMap), Request's Headers failed to be created with non-Headers object (such as array or open ended dictionary/MozMap). Before, Request's Headers could be filled with only a Headers object in Step 28. This has been expanded to accommodate array and open ended dictionary. Step 29 empties the Request's Headers list after it had been filled in Step 28, thus resulting in an empty Headers object when it shouldn't be. This step has been removed with a comment in this commit. If a RequestInit Headers is _not_ given, but a RequestInfo Headers is given, RequestInfo Headers should be used to construct Request Headers. That step has been added after Step 31. Corresponding wpt result is updated in this commit. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13758 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13802)

bors-servo · 2016-11-09T21:04:12Z

⌛ Testing commit bfd999f with merge 9a7559f...

bors-servo · 2016-11-09T21:07:19Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2016-11-09T21:41:10Z

⚡ Previous build results for mac-dev-unit are reusable. Rebuilding only arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev...

bors-servo · 2016-11-09T22:36:43Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned emilio Oct 17, 2016

highfive added the S-awaiting-review label Oct 17, 2016

emilio reviewed Oct 23, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 23, 2016

Address review comments

Loading status checks…

baacc69

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 27, 2016

KiChjang reviewed Oct 27, 2016

View changes

Remove unused use statement

Loading status checks…

e821da8

emilio approved these changes Oct 28, 2016

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 8, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 8, 2016

highfive added the S-tests-failed label Nov 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Nov 9, 2016

highfive assigned jdm and unassigned emilio Nov 9, 2016

bors-servo merged commit bfd999f into servo:master Nov 9, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 9, 2016

servo / servo

Join GitHub today

Allow Request's Headers to be created with various objects #13802

Allow Request's Headers to be created with various objects #13802

Conversation

jeenalee commented Oct 17, 2016 • edited by larsbergstrom

highfive commented Oct 17, 2016

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

emilio Oct 23, 2016 • edited by jdm

This comment has been minimized.

This comment has been minimized.

jeenalee commented Oct 24, 2016

jdm commented Oct 24, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jeenalee commented Oct 28, 2016

emilio left a comment

This comment has been minimized.

jeenalee commented Nov 7, 2016

jdm commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 8, 2016

jdm commented Nov 8, 2016

jeenalee commented Nov 8, 2016

KiChjang commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

highfive commented Nov 8, 2016

emilio commented Nov 9, 2016

bors-servo commented Nov 9, 2016

KiChjang commented Nov 9, 2016

jdm commented Nov 9, 2016

jdm commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

jeenalee commented Oct 17, 2016 •

edited by larsbergstrom

emilio Oct 23, 2016 •

edited by jdm