Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"javascript:" urls: execute in correct global scope #17083

Merged
merged 8 commits into from Sep 9, 2017

Conversation

@danielj41
Copy link
Contributor

commented May 29, 2017

Summary

This pull request makes javascript: urls execute in the correct global scope.

Example

<script> var x = 4; </script>

<!-- this branch: logs "4" -->
<!-- master: undefined variable error -->
<a href="javascript:console.log(x)">link</a>

Questions

I'm new to servo and rust, so I'm unsure about these changes. In particular:

  • What's the appropriate place to evaluate the js?
    • I moved it to handle_navigate, but I'm not sure if this will catch all occurrences of javascript: urls. I also don't know if this will execute in the correct thread and the correct window.
  • What should I do with the result of the js evaluation?
    • I just ignored it. The previous behavior displayed it as the content of a new page load.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15147, #16718
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 29, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented May 29, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/script_thread.rs
  • @KiChjang: components/script/script_thread.rs
@jdm

This comment has been minimized.

Copy link
Member

commented May 29, 2017

To answer your question about the result of execution, consider steps 9-11 of https://html.spec.whatwg.org/multipage/browsers.html#javascript-protocol. That's why the old code was treating it as the page content.

@danielj41

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

Thank you!

I think I'll need to understand the spec more thoroughly before I can figure out the correct solution. I'll take another look at this when I get a chance.

@danielj41

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2017

I'm having trouble getting this to work.

Summary

From the spec, handling javascript urls happens in the navigate algorithm, which I think corresponds to script_thread.rs's handle_navigate function. That's where I moved the js evaluation code to in this pull request. (Though, it looks like I'm also supposed to queue a task from the "DOM manipulation task source," which I haven't done).

Problem

I don't know how to implement step 12.12: "Run process a navigate response [with the result of the js evaluation]." I can't figure out a good way to skip ahead to process a navigate response, which I think corresponds to script_thread.rs's load.

Attempted solution 1

The most similar example I could find was start_page_load_about_blank, which "simulates" a response. However, copying this code requires an InProgressLoad object, which I don't think is available when handle_navigate runs. Since I can't add that object to self.incomplete_loads, the assert! in handle_page_headers_available fails.

Attempted solution 2

I also tried running the js evaluation code in pre_page_load. At this point, the InProgressLoad object is available, but I wasn't able to access the Window associated with load_data.creator_pipeline_id. I think I need this window to run the js in the correct global. When I tried find_window(), it returned None.

I'd greatly appreciate some insight from someone more familiar with Servo. Thank you for your time! :)

@danielj41 danielj41 closed this Jun 27, 2017
@jdm

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Sorry, this has been on my list of PRs that I want to give useful feedback on. I'll prioritize doing so if you're still interested in working on it!

@danielj41

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

No worries! I didn't want to clutter up the open PRs if there were more important things for you to work on.

I'm still interested in working on it. No rush, I'm fairly busy for the next week or two anyway. :)

@danielj41 danielj41 reopened this Jun 27, 2017
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

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

@jdm

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

Sorry for the extreme delay here! I kept putting off responding because I thought it would take a lot of time to wrap my head around the various complexities here, but your last comment did a really good job of laying out the issues you're facing.

I agree that neither of your attempted solutions quite fit the existing model. Here is what I recommend instead:

  • continue performing the JS evaluation in ScriptThread::handle_navigation (for the time being that's fine)
  • store the resulting body in a new field in the LoadData structure
  • in ScriptThread::handle_new_layout, obtain the body from the LoadData embedded in the NewLayoutInfo struct
  • modify ScriptThread::start_page_load_about_blank to support passing in the desired response body from the caller
  • add an extra check for javascript urls to ScriptThread::handle_new_layout that passes the body to the previously modified method

Does that make sense?

@danielj41

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2017

Thank you for taking the time to look through it!

Yes, that makes sense to me. I'll work on this a bit more when I get a chance.


I do have one follow-up question about non-string results. This is what the spec says:

  • navigate 12.11: Process result: If Type(result) is not String, then set response to a response whose status is 204.
  • process a navigate response 2: If response's status is 204 or 205, then abort these steps.

However, I didn't see any code that aborts loading a page on a 204 status (but I may have overlooked it). Which of these implementations do you think would make more sense?

  • In ScriptThread::handle_navigate, abort early if the result is not a string.
    • This seems simplest, but doesn't quite follow the spec.
  • In ScriptThread::handle_navigate, pass a 204 status through with the LoadData, then abort at a later function.
    • I'm not sure where to do this before the next page starts loading.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

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

@jdm

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

I would recommend passing the 204 status through (we can have an enum that either represents a response body or a 204 response), and then ScriptThread::start_page_load_about_blank (or an equivalent method) can adjust the Metadata value if the status code needs to change.

danielj41 added 2 commits Aug 20, 2017
Make some changes to javascript scheme url evaluation to bring it
closer to
https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigating-across-documents

- Evaluate the js before the page load so that it happens in the
  correct `window` global.
- If the result is not a string, the response should be a 204.

This required saving some data in load_data, since it's not
possible to modify the response at the point where we're evaluating
the js.
@danielj41 danielj41 force-pushed the danielj41:javascript-url-global-3 branch from 801b874 to af41769 Aug 23, 2017
@jdm jdm assigned jdm and unassigned mbrubeck Aug 24, 2017
@jdm jdm removed the S-needs-rebase label Aug 24, 2017
Copy link
Member

left a comment

This looks really great! Please be sure to check that the new test passes in Firefox too; if the test file resides in tests/wpt/web-platform-tests/html/something you can visit http://localhost:8000/html/browsers/navigating-across-documents/html/something in another browser while the test window is still open in Servo.

@@ -0,0 +1,28 @@
<!doctype html>

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

This file belongs in web-platform-tests/html/browsers/navigating-across-documents instead; tests are organized according to the layout of the specification that they are testing.

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 29, 2017

Author Contributor

Thank you for the review! Fixed in fc23cb1

@@ -600978,6 +600984,10 @@
"3dacc2783865ba292f20b72bc4c3942de521d9b0",
"support"
],
"url/a-element-href-javascript.html": [
"567d16dde294a2f3e75fdb8139d1af9a8c73e0fc",

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

Be sure to run ./mach update-manifest after moving the test (and making any changes to it).

document.querySelector("#javascript-link").click();
step_timeout(function() {

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

Let's do this instead:

<script>
function changeStatus() {
  t.done();
}

var t = async_test(function(t) {
  document.querySelector("#javascript-link").click();
}, "javascript: scheme urls should be executed in current global scope");
</script>

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 29, 2017

Author Contributor

Fixed in fc23cb1

}
None => {
let is_javascript = load_data.url.scheme() == "javascript";
if is_javascript {
use url::percent_encoding::percent_decode;

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

Move this to with the other use statements at the top of the file.

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 29, 2017

Author Contributor

Fixed in 5d28dd6

replace: bool) {
match browsing_context_id {
Some(browsing_context_id) => {
let iframe = self.documents.borrow().find_iframe(parent_pipeline_id, browsing_context_id);
if let Some(iframe) = iframe {
iframe.navigate_or_reload_child_browsing_context(Some(load_data), NavigationType::Regular, replace);
}

// TODO: Test javascript: urls in iframes

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

This looks more like a "note to self"; want to file an issue for this and remove the comment instead?

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 25, 2017

Author Contributor

I was planning on looking into it (and this TODO) a bit more in this pull request. I'll open new issues if they aren't easy to fix!

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 29, 2017

Author Contributor

I added support for <iframe src="javascript:'something'"></iframe> in 6ae6031. This was necessary to prevent a regression from master.

I fixed the other TODO (can't follow links after executing a javascript url) in ff786a0.

}

if is_javascript {
load_data.url = ServoUrl::parse("about:blank").unwrap();

This comment has been minimized.

Copy link
@jdm

jdm Aug 24, 2017

Member

This block can be merged with the previous one.

This comment has been minimized.

Copy link
@danielj41

danielj41 Aug 29, 2017

Author Contributor

I tried to make this look cleaner in 5d28dd6. I couldn't merge it with the previous block because there was an immutable reference to url in that block (though I suppose I could change that).

@jdm

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

  ▶ OK [expected TIMEOUT] /XMLHttpRequest/open-url-javascript-window-2.htm

  ▶ Unexpected subtest result in /XMLHttpRequest/open-url-javascript-window-2.htm:
  │ FAIL [expected TIMEOUT] XMLHttpRequest: open() - resolving URLs (javascript: <iframe>; 2)
  └   → The string did not match the expected pattern.

  /XMLHttpRequest/open-url-multi-window-3.htm
  ▶ OK [expected TIMEOUT] /XMLHttpRequest/open-url-javascript-window.htm

  ▶ Unexpected subtest result in /XMLHttpRequest/open-url-javascript-window.htm:
  └ PASS [expected TIMEOUT] XMLHttpRequest: open() - resolving URLs (javascript: <iframe>; 1)

  ▶ Unexpected subtest result in /dom/nodes/Document-contentType/contentType/contenttype_javascripturi.html:
  └ PASS [expected FAIL] Javascript URI document.contentType === 'text/html'

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/013.html:
  └ PASS [expected FAIL] Link with onclick navigation to javascript url with delayed document.write and href navigation 

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/014.html:
  └ PASS [expected FAIL]  Link with javascript onclick form submission script order 

  /fetch/http-cache/status.html
  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/015.html:
  └ PASS [expected FAIL]  Link with javascript onclick and href script order 

  /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-global-scope.html
  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html:
  └ PASS [expected FAIL] iframes with javascript src

  ▶ Unexpected subtest result in /html/dom/documents/dom-tree-accessors/Document.currentScript.html:
  └ PASS [expected NOTRUN] Script iframe-src

  ▶ Unexpected subtest result in /old-tests/submission/Opera/script_scheduling/028.html:
  └ PASS [expected FAIL]  scheduler: javascript: URL

  ▶ Unexpected subtest result in /old-tests/submission/Opera/script_scheduling/029.html:
  └ PASS [expected FAIL]  scheduler: javascript: URL in HREF

Looks like a bunch of test expectations can be updated to reflect the fact that they now pass (or at least don't timeout any longer).

@KiChjang

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

@danielj41 By default, WPTs are expected to PASS or OK, so if there is an expectation INI file that expects tests to FAIL before but is now passing, you may simply delete the lines, or remove the expectation file altogether if there's only one test.

@danielj41 danielj41 force-pushed the danielj41:javascript-url-global-3 branch from cab36ef to 452db05 Sep 9, 2017
@danielj41

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

@KiChjang That makes more sense, thanks! I amended the commit.

@KiChjang

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2017

📌 Commit 452db05 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2017

⌛️ Testing commit 452db05 with merge 40c8a63...

bors-servo added a commit that referenced this pull request Sep 9, 2017
"javascript:" urls: execute in correct global scope

<!-- Please describe your changes on the following line: -->

#### Summary

This pull request makes `javascript:` urls execute in the correct global scope.

#### Example

```html
<script> var x = 4; </script>

<!-- this branch: logs "4" -->
<!-- master: undefined variable error -->
<a href="javascript:console.log(x)">link</a>
```

#### Questions

I'm new to servo and rust, so I'm unsure about these changes. In particular:
  * What's the appropriate place to evaluate the js?
    * I moved it to `handle_navigate`, but I'm not sure if this will catch all occurrences of `javascript:` urls. I also don't know if this will execute in the correct thread and the correct window.
  * What should I do with the result of the js evaluation?
    * I just ignored it. The previous behavior displayed it as the content of a new page load.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15147, #16718

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17083)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2017

@bors-servo bors-servo merged commit 452db05 into servo:master Sep 9, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
jdm added a commit to web-platform-tests/wpt that referenced this pull request Oct 6, 2017
jdm added a commit to web-platform-tests/wpt that referenced this pull request Oct 6, 2017
- move test to correct directory
- update it to be more concise

I also confirmed that this test passes in Firefox.

Upstreamed from servo/servo#17083 [ci skip]
jdm added a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2017
jdm added a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2017
- move test to correct directory
- update it to be more concise

I also confirmed that this test passes in Firefox.

Upstreamed from servo/servo#17083 [ci skip]
jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
- move test to correct directory
- update it to be more concise

I also confirmed that this test passes in Firefox.

Upstreamed from servo/servo#17083 [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.