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

Report meaningful line numbers for inline script errors #14963

Merged
merged 1 commit into from Jan 12, 2017
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Store parser's current line when script elements are created.

Use the newly stored line as the starting line number when
evaluating JS. This ensures that inline scripts will report
errors with meaningful line numbers.
  • Loading branch information
karenher authored and jdm committed Jan 12, 2017
commit db2082bc6e0edc0028f287d4acc203e7c3bc829f
@@ -145,10 +145,25 @@ impl fmt::Debug for Element {

#[derive(PartialEq, HeapSizeOf)]
pub enum ElementCreator {
ParserCreated,
ParserCreated(u64),
ScriptCreated,
}

impl ElementCreator {
pub fn is_parser_created(&self) -> bool {
match *self {
ElementCreator::ParserCreated(_) => true,
ElementCreator::ScriptCreated => false,
}
}
pub fn return_line_number(&self) -> u64 {
match *self {
ElementCreator::ParserCreated(l) => l,
ElementCreator::ScriptCreated => 1,
}
}
}

pub enum AdjacentPosition {
BeforeBegin,
AfterEnd,
@@ -339,13 +339,13 @@ impl GlobalScope {
/// Evaluate JS code on this global scope.
pub fn evaluate_js_on_global_with_result(
&self, code: &str, rval: MutableHandleValue) {
self.evaluate_script_on_global_with_result(code, "", rval)
self.evaluate_script_on_global_with_result(code, "", rval, 1)
}

/// Evaluate a JS script on this global scope.
#[allow(unsafe_code)]
pub fn evaluate_script_on_global_with_result(
&self, code: &str, filename: &str, rval: MutableHandleValue) {
&self, code: &str, filename: &str, rval: MutableHandleValue, line_number: u32) {
let metadata = time::TimerMetadata {
url: if filename.is_empty() {
self.get_url().as_str().into()
@@ -367,7 +367,7 @@ impl GlobalScope {

let _ac = JSAutoCompartment::new(cx, globalhandle.get());
let _aes = AutoEntryScript::new(self);
let options = CompileOptionsWrapper::new(cx, filename.as_ptr(), 1);
let options = CompileOptionsWrapper::new(cx, filename.as_ptr(), line_number);
unsafe {
if !Evaluate2(cx, options.ptr, code.as_ptr(),
code.len() as libc::size_t,
@@ -59,7 +59,7 @@ impl HTMLLinkElement {
HTMLLinkElement {
htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
rel_list: Default::default(),
parser_inserted: Cell::new(creator == ElementCreator::ParserCreated),
parser_inserted: Cell::new(creator.is_parser_created()),
stylesheet: DOMRefCell::new(None),
cssom_stylesheet: MutNullableJS::new(None),
pending_loads: Cell::new(0),
@@ -60,6 +60,9 @@ pub struct HTMLScriptElement {

/// Document of the parser that created this element
parser_document: JS<Document>,

/// Track line line_number
line_number: u64,
}

impl HTMLScriptElement {
@@ -69,10 +72,11 @@ impl HTMLScriptElement {
htmlelement:
HTMLElement::new_inherited(local_name, prefix, document),
already_started: Cell::new(false),
parser_inserted: Cell::new(creator == ElementCreator::ParserCreated),
non_blocking: Cell::new(creator != ElementCreator::ParserCreated),
parser_inserted: Cell::new(creator.is_parser_created()),
non_blocking: Cell::new(!creator.is_parser_created()),
ready_to_be_parser_executed: Cell::new(false),
parser_document: JS::from_ref(document),
line_number: creator.return_line_number(),
}
}

@@ -508,7 +512,7 @@ impl HTMLScriptElement {
let window = window_from_node(self);
rooted!(in(window.get_cx()) let mut rval = UndefinedValue());
window.upcast::<GlobalScope>().evaluate_script_on_global_with_result(
&script.text, script.url.as_str(), rval.handle_mut());
&script.text, script.url.as_str(), rval.handle_mut(), self.line_number as u32);

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jan 11, 2017

Member

Can we avoid this cast?

This comment has been minimized.

Copy link
@jdm

jdm Jan 11, 2017

Author Member

We made the line numbers u64 in html5ever, so there's going to be a cast somewhere until we change that.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jan 11, 2017

Member

Oh well.


// Step 6.
document.set_current_script(old_script.r());
@@ -50,8 +50,8 @@ impl HTMLStyleElement {
htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
stylesheet: DOMRefCell::new(None),
cssom_stylesheet: MutNullableJS::new(None),
parser_inserted: Cell::new(creator == ElementCreator::ParserCreated),
in_stack_of_open_elements: Cell::new(creator == ElementCreator::ParserCreated),
parser_inserted: Cell::new(creator.is_parser_created()),
in_stack_of_open_elements: Cell::new(creator.is_parser_created()),
pending_loads: Cell::new(0),
any_failed_load: Cell::new(false),
}
@@ -52,6 +52,7 @@ impl Tokenizer {
let sink = Sink {
base_url: url,
document: JS::from_ref(document),
current_line: 1,
};

let options = TreeBuilderOpts {
@@ -122,6 +123,7 @@ unsafe impl JSTraceable for HtmlTokenizer<TreeBuilder<JS<Node>, Sink>> {
struct Sink {
base_url: ServoUrl,
document: JS<Document>,
current_line: u64,
}

impl TreeSink for Sink {
@@ -156,7 +158,7 @@ impl TreeSink for Sink {
fn create_element(&mut self, name: QualName, attrs: Vec<Attribute>)
-> JS<Node> {
let elem = Element::create(name, None, &*self.document,
ElementCreator::ParserCreated);
ElementCreator::ParserCreated(self.current_line));

for attr in attrs {
elem.set_attribute_from_parser(attr.name, DOMString::from(String::from(attr.value)), None);
@@ -234,6 +236,10 @@ impl TreeSink for Sink {
}
}

fn set_current_line(&mut self, line_number: u64) {
self.current_line = line_number;
}

fn pop(&mut self, node: JS<Node>) {
let node = Root::from_ref(&*node);
vtable_for(&node).pop();
@@ -134,8 +134,9 @@ impl TreeSink for Sink {
ns: name.namespace_url,
local: name.local,
};
//TODO: Add ability to track lines to API of xml5ever
let elem = Element::create(name, prefix, &*self.document,
ElementCreator::ParserCreated);
ElementCreator::ParserCreated(1));

for attr in attrs {
let name = QualName {

This file was deleted.

This file was deleted.

"url": "/_mozilla/mozilla/trace_null.html"
}
],
"mozilla/track_line.html": [
{
"path": "mozilla/track_line.html",
"url": "/_mozilla/mozilla/track_line.html"
}
],
"mozilla/union.html": [
{
"path": "mozilla/union.html",
@@ -0,0 +1,17 @@
<!doctype html>
<meta charset="utf-8">
<title>Test that errors from inline scripts report meaningful line numbers</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({allow_uncaught_exception:true});
var t = async_test("error event has proper line number");
window.addEventListener('error', t.step_func(function(e) {
assert_true(e instanceof ErrorEvent);
assert_equals(e.lineno, 16);
t.done();
}), true);
</script>
<script>
this_is_a_js_error
</script>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.