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
124 changes: 80 additions & 44 deletions components/script/script_thread.rs
Expand Up @@ -87,8 +87,8 @@ use script_runtime::{CommonScriptMsg, ScriptChan, ScriptThreadEventCategory};
use script_runtime::{ScriptPort, StackRootTLS, get_reports, new_rt_and_cx};
use script_traits::{CompositorEvent, ConstellationControlMsg};
use script_traits::{DocumentActivity, DiscardBrowsingContext, EventResult};
use script_traits::{InitialScriptState, LayoutMsg, LoadData, MouseButton, MouseEventType, MozBrowserEvent};
use script_traits::{NewLayoutInfo, ScriptToConstellationChan, ScriptMsg, UpdatePipelineIdReason};
use script_traits::{InitialScriptState, JsEvalResult, LayoutMsg, LoadData, MouseButton, MouseEventType};
use script_traits::{MozBrowserEvent, NewLayoutInfo, ScriptToConstellationChan, ScriptMsg, UpdatePipelineIdReason};
use script_traits::{ScriptThreadFactory, TimerEvent, TimerSchedulerMsg, TimerSource};
use script_traits::{TouchEventType, TouchId, UntrustedNodeAddress, WindowSizeData, WindowSizeType};
use script_traits::CompositorEvent::{KeyEvent, MouseButtonEvent, MouseMoveEvent, ResizeEvent};
Expand Down Expand Up @@ -1506,7 +1506,7 @@ impl ScriptThread {
load_data.url.clone(),
origin);
if load_data.url.as_str() == "about:blank" {
self.start_page_load_about_blank(new_load);
self.start_page_load_about_blank(new_load, load_data.js_eval_result);
} else {
self.pre_page_load(new_load, load_data);
}
Expand Down Expand Up @@ -1677,6 +1677,18 @@ impl ScriptThread {
match idx {
Some(idx) => {
let load = self.incomplete_loads.borrow_mut().remove(idx);

// https://html.spec.whatwg.org/multipage/#process-a-navigate-response
// 2. If response's status is 204 or 205, then abort these steps.
match metadata {
Some(Metadata { status: Some((204 ... 205, _)), .. }) => {
// TODO: This leaves the page in a broken state where you can't follow
// other links. Fix this.
return None;
},
_ => ()
};

metadata.map(|meta| self.load(meta, load))
}
None => {
Expand Down Expand Up @@ -2116,39 +2128,7 @@ impl ScriptThread {
// Notify devtools that a new script global exists.
self.notify_devtools(document.Title(), final_url.clone(), (incomplete.pipeline_id, None));

let is_javascript = incomplete.url.scheme() == "javascript";
let parse_input = if is_javascript {
use url::percent_encoding::percent_decode;

// Turn javascript: URL into JS code to eval, according to the steps in
// https://html.spec.whatwg.org/multipage/#javascript-protocol

// This slice of the URL’s serialization is equivalent to (5.) to (7.):
// Start with the scheme data of the parsed URL;
// append question mark and query component, if any;
// append number sign and fragment component if any.
let encoded = &incomplete.url[Position::BeforePath..];

// Percent-decode (8.) and UTF-8 decode (9.)
let script_source = percent_decode(encoded.as_bytes()).decode_utf8_lossy();

// Script source is ready to be evaluated (11.)
unsafe {
let _ac = JSAutoCompartment::new(self.get_cx(), window.reflector().get_jsobject().get());
rooted!(in(self.get_cx()) let mut jsval = UndefinedValue());
window.upcast::<GlobalScope>().evaluate_js_on_global_with_result(
&script_source, jsval.handle_mut());
let strval = DOMString::from_jsval(self.get_cx(),
jsval.handle(),
StringificationBehavior::Empty);
match strval {
Ok(ConversionResult::Success(s)) => s,
_ => DOMString::new(),
}
}
} else {
DOMString::new()
};
let parse_input = DOMString::new();

document.set_https_state(metadata.https_state);

Expand Down Expand Up @@ -2327,16 +2307,64 @@ impl ScriptThread {
/// for the given pipeline (specifically the "navigate" algorithm).
fn handle_navigate(&self, parent_pipeline_id: PipelineId,
browsing_context_id: Option<BrowsingContextId>,
load_data: LoadData,
mut load_data: LoadData,
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@archive64 archive64 Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d28dd6


// Turn javascript: URL into JS code to eval, according to the steps in
// https://html.spec.whatwg.org/multipage/#javascript-protocol

// This slice of the URL’s serialization is equivalent to (5.) to (7.):
// Start with the scheme data of the parsed URL;
// append question mark and query component, if any;
// append number sign and fragment component if any.
let encoded = &load_data.url[Position::BeforePath..];

// Percent-decode (8.) and UTF-8 decode (9.)
let script_source = percent_decode(encoded.as_bytes()).decode_utf8_lossy();

// Script source is ready to be evaluated (11.)
let window = self.documents.borrow().find_window(parent_pipeline_id);
if let Some(window) = window {
let _ac = JSAutoCompartment::new(self.get_cx(), window.reflector().get_jsobject().get());
rooted!(in(self.get_cx()) let mut jsval = UndefinedValue());
window.upcast::<GlobalScope>().evaluate_js_on_global_with_result(
&script_source, jsval.handle_mut());

load_data.js_eval_result = if jsval.get().is_string() {
unsafe {
let strval = DOMString::from_jsval(self.get_cx(),
jsval.handle(),
StringificationBehavior::Empty);
match strval {
Ok(ConversionResult::Success(s)) => {
Some(JsEvalResult::Ok(String::from(s).as_bytes().to_vec()))
},
_ => None,
}
}
} else {
Some(JsEvalResult::NoContent)
};
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block can be merged with the previous one.

Copy link
Contributor Author

@archive64 archive64 Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

self.script_sender
.send((parent_pipeline_id, ScriptMsg::LoadUrl(load_data, replace)))
.unwrap();
Expand Down Expand Up @@ -2375,7 +2403,7 @@ impl ScriptThread {
/// argument until a notification is received that the fetch is complete.
fn pre_page_load(&self, incomplete: InProgressLoad, load_data: LoadData) {
let id = incomplete.pipeline_id.clone();
let mut req_init = RequestInit {
let req_init = RequestInit {
url: load_data.url.clone(),
method: load_data.method,
destination: Destination::Document,
Expand All @@ -2391,10 +2419,6 @@ impl ScriptThread {
.. RequestInit::default()
};

if req_init.url.scheme() == "javascript" {
req_init.url = ServoUrl::parse("about:blank").unwrap();
}

let context = ParserContext::new(id, load_data.url);
self.incomplete_parser_contexts.borrow_mut().push((id, context));

Expand Down Expand Up @@ -2434,7 +2458,7 @@ impl ScriptThread {

/// Synchronously fetch `about:blank`. Stores the `InProgressLoad`
/// argument until a notification is received that the fetch is complete.
fn start_page_load_about_blank(&self, incomplete: InProgressLoad) {
fn start_page_load_about_blank(&self, incomplete: InProgressLoad, js_eval_result: Option<JsEvalResult>) {
let id = incomplete.pipeline_id;

self.incomplete_loads.borrow_mut().push(incomplete);
Expand All @@ -2444,8 +2468,20 @@ impl ScriptThread {

let mut meta = Metadata::default(url);
meta.set_content_type(Some(&mime!(Text / Html)));

// If this page load is the result of a javascript scheme url, map
// the evaluation result into a response.
let chunk = match js_eval_result {
Some(JsEvalResult::Ok(content)) => content,
Some(JsEvalResult::NoContent) => {
meta.status = Some((204, b"No Content".to_vec()));
vec![]
},
None => vec![]
};

context.process_response(Ok(FetchMetadata::Unfiltered(meta)));
context.process_response_chunk(vec![]);
context.process_response_chunk(chunk);
context.process_response_eof(Ok(()));
}

Expand Down
13 changes: 13 additions & 0 deletions components/script_traits/lib.rs
Expand Up @@ -146,12 +146,24 @@ pub struct LoadData {
pub headers: Headers,
/// The data.
pub data: Option<Vec<u8>>,
/// The result of evaluating a javascript scheme url.
pub js_eval_result: Option<JsEvalResult>,
/// The referrer policy.
pub referrer_policy: Option<ReferrerPolicy>,
/// The referrer URL.
pub referrer_url: Option<ServoUrl>,
}

/// The result of evaluating a javascript scheme url.
#[derive(Clone, Debug, Deserialize, Serialize)]
pub enum JsEvalResult {
/// The js evaluation had a non-string result, 204 status code.
/// https://html.spec.whatwg.org/multipage/#navigate 12.11
NoContent,
/// The js evaluation had a string result.
Ok(Vec<u8>)
}

impl LoadData {
/// Create a new `LoadData` object.
pub fn new(url: ServoUrl,
Expand All @@ -165,6 +177,7 @@ impl LoadData {
method: Method::Get,
headers: Headers::new(),
data: None,
js_eval_result: None,
referrer_policy: referrer_policy,
referrer_url: referrer_url,
}
Expand Down
10 changes: 10 additions & 0 deletions tests/wpt/metadata/MANIFEST.json
Expand Up @@ -359897,6 +359897,12 @@
{}
]
],
"url/a-element-href-javascript.html": [
[
"/url/a-element-href-javascript.html",
{}
]
],
"url/a-element-origin-xhtml.xhtml": [
[
"/url/a-element-origin-xhtml.xhtml",
Expand Down Expand Up @@ -600978,6 +600984,10 @@
"3dacc2783865ba292f20b72bc4c3942de521d9b0",
"support"
],
"url/a-element-href-javascript.html": [
"567d16dde294a2f3e75fdb8139d1af9a8c73e0fc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"testharness"
],
"url/a-element-origin-xhtml.xhtml": [
"56019fd2d3870324ba412e3e0c602bad3b90ef49",
"testharness"
Expand Down
28 changes: 28 additions & 0 deletions tests/wpt/web-platform-tests/url/a-element-href-javascript.html
@@ -0,0 +1,28 @@
<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@archive64 archive64 Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review! Fixed in fc23cb1

<meta charset=utf-8>
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>

<script>
var linkStatus = 'link not clicked';

function changeStatus() {
linkStatus = 'link has been clicked';
}
</script>

<a id="javascript-link" href="javascript:changeStatus()">link</a>

<script>
setup({explicit_done:true});

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

step_timeout(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fc23cb1

test(function() {
assert_equals(linkStatus, "link has been clicked");
}, "javascript: scheme urls should be executed in correct global scope");

done();
});
</script>