Redirect document loads manually #15354

cynicaldevil · 2017-02-02T18:27:30Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14596 .

r? @jdm

I ran some tests at random from the navigating-across-documents folder, and they are passing.

This change is

highfive · 2017-02-02T18:27:34Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/Cargo.toml, components/constellation/constellation.rs, components/constellation/lib.rs
@fitzgen: components/script/script_thread.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@KiChjang: components/net/resource_thread.rs, components/net/http_loader.rs, components/script/script_thread.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/net_traits/response.rs, components/net_traits/response.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2017-02-02T18:27:35Z

Warning

These commits modify net and script code, but no tests are modified. Please consider adding a test!

jdm · 2017-02-02T19:19:22Z

0.04s$ bash etc/ci/check_no_panic.sh

components/compositing/compositor.rs

ports/glutin/lib.rs

ports/glutin/window.rs

components/constellation/:

Cargo.toml

constellation.rs

event_loop.rs

frame.rs

lib.rs

pipeline.rs

sandboxing.rs

timer_scheduler.rs

components/constellation/constellation.rs:1411:            let (sender, receiver) = ipc::channel().unwrap();

components/constellation/constellation.rs:1414:                                              sender)).unwrap();

components/constellation/constellation.rs:1419:                            match res_metadata.unwrap() {

components/constellation/constellation.rs:1437:                                            req_init.url = ServoUrl::parse(url).unwrap();

The command "bash etc/ci/check_no_panic.sh" exited with 1.

cynicaldevil · 2017-02-03T15:11:43Z

@jdm Fixed.

KiChjang · 2017-02-03T19:41:19Z

components/constellation/constellation.rs

+                        FetchResponseMsg::ProcessResponse(res_metadata) => {
+                            match res_metadata {
+                                Ok(metadata) => {
+                                    match metadata {


It looks very much like you can combine the 4 matches above into a single match.

Yeah, I thought there might be a better method to do this, but I went for the easy route anyway :P I'll look into it.

jdm

This is a good start! The conversion to process the response asynchronously is the biggest issue here; I recommend taking a look at how the ParserContext and NetworkListener types work, since that's what the script crate uses for the same purpose

jdm · 2017-02-03T20:35:01Z

components/constellation/constellation.rs

+                                sender)) {
+                warn!("Exit resource thread failed ({})", e);
+            }
+            match receiver.recv() {


One problem here is that this is a synchronous interaction with the network thread. This means that the constellation will not be able to process anything else besides a single network request at a time, which will be very inefficient. We should be using a mechanism like script's NetworkListener, which receives notifications on the network thread and forwards them to the script thread's event loop via a sender. This means that the constellation will need to keep track of in-progress loads so it can know how to deal with them from the regular event loop.

jdm · 2017-02-03T20:35:01Z

components/script_traits/lib.rs

@@ -197,6 +198,9 @@ pub enum DiscardBrowsingContext {
 /// Messages sent from the constellation or layout to the script thread.
 #[derive(Deserialize, Serialize)]
 pub enum ConstellationControlMsg {
+    /// Sends the final request to script thread for fetching after all redirections
+    /// have been resolved
+    FinalRequest(PipelineId, RequestInit),


This will be able to contain a Result<FetchMetadata, ...> instead of RequestInit if the constellation sends the final response header received instead. We can call it NavigationResponse instead

jdm · 2017-02-03T20:35:01Z

components/script_traits/script_msg.rs

@@ -66,6 +66,9 @@ pub enum LogEntry {
 /// Messages from the script to the constellation.
 #[derive(Deserialize, Serialize)]
 pub enum ScriptMsg {
+    /// Requests are sent to constellation and fetches are checked manually
+    /// for cross-origin loads
+    ManualRedirect(PipelineId, LoadData),


A more fitting name would be InitiateNavigateRequest.

jdm · 2017-02-03T20:35:01Z

components/script/script_thread.rs

-        let context = Arc::new(Mutex::new(ParserContext::new(id, load_data.url.clone())));
+    /// Initiate a non-blocking fetch for a specified resource.
+    fn start_page_load(&self, id: PipelineId, mut req_init: RequestInit) {
+        let context = Arc::new(Mutex::new(ParserContext::new(id, req_init.url.clone())));


This is where we should be able to invoke the FetchResponseListener methods using the data provided by the constellation.

jdm · 2017-02-03T20:35:01Z

components/script/script_thread.rs

@@ -2073,12 +2075,17 @@ impl ScriptThread {
        window.evaluate_media_queries_and_report_changes();
    }

-    /// Initiate a non-blocking fetch for a specified resource. Stores the InProgressLoad
+    /// Sends a message to constellation to check for cross-origin redirects. Stores the InProgressLoad


/// Instructs the constellation to fetch the document that will be loaded. Stores the ...

jdm · 2017-02-03T20:35:01Z

components/net_traits/response.rs

@@ -75,6 +75,19 @@ pub enum ResponseMsg {
    Errored,
 }

+#[derive(Serialize, Deserialize, Clone)]
+pub struct ResponseInit {
+    pub url: ServoUrl,


We will need more data here; for example, http_redirect_fetch looks at the headers of the response, and those will be initialized as empty in this case. The FetchMetadata type is serializable and contains everything we should need, I believe.

jdm · 2017-02-03T20:35:01Z

components/net_traits/response.rs

+    pub url: ServoUrl,
+}
+
+impl Default for ResponseInit {


Why do we need this implementation?

jdm · 2017-02-03T20:35:01Z

components/constellation/constellation.rs

+            };
+        }
+
+        let msg = ConstellationControlMsg::FinalRequest(id, req_init);


I'm not looking at the rest of the code in this method, because I believe that restructuring it to use an async fetch listener will change it significantly.

jdm · 2017-02-03T20:35:01Z

components/constellation/constellation.rs

+                        _ => break
+                    };
+                    match data {
+                        Some((FilteredMetadata::OpaqueRedirect, _)) => {


We will need to deal with non-cross-origin redirects as well, which won't be tagged as OpaqueRedirect.

jdm · 2017-02-03T20:35:01Z

components/constellation/constellation.rs

+                                match unsafe_metadata.headers {
+                                    Some(headers) => match headers.get::<Location>() {
+                                        Some(url) => {
+                                            match ServoUrl::parse(url) {


We should not need to process the location header at all; that's handled by http_redirect_fetch. We only need to check if the header is present.

@jdm But how else do I extract the url to modify req_init to be sent to resource_thread?

Why does req_init need to be modified? Send the original one along with the response.

Although it occurs to me that that would break the following case:

fetch http://foo.com

redirect to http://bar.com

redirect to /foo.html

However, the response contains a url list, so it should be possible to parse the location value using the most recent entry in the url list, and progressively move backwards until a successfull URL is parsed.

cynicaldevil

@jdm I've implemented the listener code and also added other small changes, but I'm not sure if this is what you had in mind :)

cynicaldevil · 2017-02-14T19:14:56Z

components/constellation/network_listener.rs

+}
+
+impl NetworkListener {
+    pub fn notify(&self, message: Result<FetchMetadata, NetworkError>) {


I stole most of the names from script/network_listener.rs, since I'm not very good at naming things :P

cynicaldevil · 2017-02-14T19:14:56Z

components/constellation/network_listener.rs

+                                Some(headers) => {
+                                    if headers.has::<Location>() {
+                                        let (sender, _) = ipc::channel().expect("Failed to create IPC channel!");
+                                        result = Some(sender.send(ScriptMsg::InitiateNavigateRequest(


I send a message back to constellation so that the process starts all over again from here.

bors-servo · 2017-02-16T12:50:21Z

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

jdm · 2017-02-17T20:28:29Z

Have you tried running this on any pages? As far as I can tell, there's no code that propagates the response body from the constellation to the script thread, so no pages will end up being parsed. Additionally, I would expect the message conversion code in the router listener to panic.

cynicaldevil · 2017-02-18T10:48:07Z

@jdm Hmm...so right now I am only processing headers by acknowledging messages of type ProcessResponse, but when I receive a non-redirecting response, I should also intercept a ProcessResponseEOF type message, right?

jdm · 2017-02-18T13:48:45Z

All of the FetchResponseMsg messages will be send to the constellation for every response, whether it redirects or not.

cynicaldevil · 2017-02-18T20:25:08Z

weird, the closure inside the router listener is never being called, although I'm pretty sure that the message is being sent. Am I using the router listener correctly?

Edit: nvm, the response I'm sending doesn't have any location headers set and therefore http_redirect_fetch is returning early. Hence this :P

bors-servo · 2017-02-18T21:28:39Z

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

cynicaldevil · 2017-02-20T18:05:38Z

I don't see how we can do async fetches if we want to use http_redirect_fetch as well, because internally, it uses a main fetch with the recursive flag set, which means main fetch will return the response early, before the FetchTaskTarget functions are called.

jdm · 2017-02-24T15:42:52Z

Let's duplicate the code inside main fetch step 19 before returning early in step 13. I've filed whatwg/html#2396 about resolving this issue in the specification.

jdm · 2017-02-24T16:21:52Z

Actually, that issue points out that we could probably address this by not setting the recursive flag when invoking the http-redirect fetch algorithm as a top-level algorithm as we do in this PR.

cynicaldevil · 2017-05-31T10:36:11Z

@bors-servo try

bors-servo · 2017-05-31T10:36:15Z

⌛ Trying commit ea59b0b with merge bf3335b...


        Auto merge of #15354 - cynicaldevil:manual-redirect, r=<try>

@jdm

Redirect document loads manually  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14596 . r? @jdm I ran some tests at random from the `navigating-across-documents` folder, and they are passing.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15354)

bors-servo · 2017-05-31T10:36:30Z

💔 Test failed - linux-rel-wpt

cynicaldevil · 2017-05-31T11:20:42Z

@bors-servo try

bors-servo · 2017-05-31T11:20:46Z

⌛ Trying commit bb8db03 with merge 1085653...


        Auto merge of #15354 - cynicaldevil:manual-redirect, r=<try>

@jdm

Redirect document loads manually  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14596 . r? @jdm I ran some tests at random from the `navigating-across-documents` folder, and they are passing.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15354)

bors-servo · 2017-05-31T11:39:05Z

💔 Test failed - linux-dev


        Redirect document loads manually

cynicaldevil · 2017-05-31T13:00:32Z

@jdm Ready for merge now.

jdm · 2017-06-02T21:31:57Z

@bors-servo: r+

bors-servo · 2017-06-02T21:31:59Z

📌 Commit 541baaf has been approved by jdm

bors-servo · 2017-06-02T21:49:23Z

⌛ Testing commit 541baaf with merge eac4f40...


        Auto merge of #15354 - cynicaldevil:manual-redirect, r=jdm

@jdm

Redirect document loads manually  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14596 . r? @jdm I ran some tests at random from the `navigating-across-documents` folder, and they are passing.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15354)

bors-servo · 2017-06-02T22:43:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing eac4f40 to master...

highfive assigned jdm Feb 2, 2017

highfive added the S-awaiting-review label Feb 2, 2017

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch 2 times, most recently from 5430cd9 to f12f783 Feb 3, 2017

KiChjang reviewed Feb 3, 2017

View changes

jdm requested changes Feb 3, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 3, 2017

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch from f12f783 to fe06513 Feb 14, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 14, 2017

cynicaldevil reviewed Feb 14, 2017

View changes

highfive added the S-needs-rebase label Feb 16, 2017

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch from fe06513 to 424b194 Feb 16, 2017

highfive added the S-needs-rebase label Feb 18, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 27, 2017

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch from cc2b6be to ea59b0b May 31, 2017

highfive added S-awaiting-review and removed S-tests-failed labels May 31, 2017

highfive added the S-tests-failed label May 31, 2017

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch from ea59b0b to bb8db03 May 31, 2017

highfive removed the S-tests-failed label May 31, 2017

highfive added the S-tests-failed label May 31, 2017

Redirect document loads manually

Loading status checks…

541baaf

cynicaldevil force-pushed the cynicaldevil:manual-redirect branch from bb8db03 to 541baaf May 31, 2017

highfive removed the S-tests-failed label May 31, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 2, 2017

bors-servo merged commit 541baaf into servo:master Jun 2, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 2, 2017

bors-servo mentioned this pull request Jun 2, 2017

Properly set origin of fetch requests #16508

Merged

4 of 5 tasks complete

cynicaldevil deleted the cynicaldevil:manual-redirect branch Jun 4, 2017

servo / servo

Join GitHub today

Redirect document loads manually #15354

Redirect document loads manually #15354

Conversation

cynicaldevil commented Feb 2, 2017 • edited

highfive commented Feb 2, 2017

highfive commented Feb 2, 2017

jdm commented Feb 2, 2017

cynicaldevil commented Feb 3, 2017

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

jdm Feb 10, 2017 • edited

cynicaldevil left a comment

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 16, 2017

jdm commented Feb 17, 2017

cynicaldevil commented Feb 18, 2017

jdm commented Feb 18, 2017

cynicaldevil commented Feb 18, 2017 • edited

bors-servo commented Feb 18, 2017

cynicaldevil commented Feb 20, 2017

jdm commented Feb 24, 2017

jdm commented Feb 24, 2017

cynicaldevil commented May 31, 2017

bors-servo commented May 31, 2017

bors-servo commented May 31, 2017

cynicaldevil commented May 31, 2017

bors-servo commented May 31, 2017

bors-servo commented May 31, 2017

cynicaldevil commented May 31, 2017

jdm commented Jun 2, 2017

bors-servo commented Jun 2, 2017

bors-servo commented Jun 2, 2017

bors-servo commented Jun 2, 2017

cynicaldevil commented Feb 2, 2017 •

edited

jdm Feb 10, 2017 •

edited

cynicaldevil commented Feb 18, 2017 •

edited