jdm
commented
7 years ago As an example of how Chrome's behaviour (which this patch implements with more accuracy) can go wrong:
<!doctype html>
<html>
<head>
<title>window.onerror - runtime error in compiled event handler</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
</head>
<body>
<div id=log></div>
<script>
setup({allow_uncaught_exception:true});
var t = async_test();
addEventListener('error', t.step_func_done(function(e) {
assert_true(e instanceof ErrorEvent);
assert_equals(e.filename, location.href);
assert_equals(e.lineno, 4);
}));
</script>
<div id="eventtarget"></div>
<script>
t.step(function() {
var div = document.querySelector('#eventtarget');
div.setAttribute('onclick', '\n\n\nundefined_variable');
div.dispatchEvent(new Event('click'));
});
</script>
</body>
</html>In Chrome, this reports an error on line 29 of the HTML file.
Ms2ger
This is a patch that I wrote which makes the following code easier to debug:
Without that patch, the error is reported as occurring on line N of the page that is evaluating the setAttribute call, where N is the line number inside the inline JS that triggers the exception (which is often 1 since there are rarely line breaks). With the patch, the error is reported as occurring on line M+N of the file that contains the setAttribute call, where M is the line number that contains the setAttribute call, which means it's much easier to relate the exception to the actual code that threw it.
However! This is not clearly an improvement in all cases, because it could appear that the setAttribute call is the source of the exception. Additionally, consider the following:
This patch is only a marginal improvement improvement in cases where the inline JS source is not present in the line that is reported as the cause, because it still requires backtracking.
A WPT test of this case reveals that Firefox chooses to report the line starting at 1, while Chrome appears to try to report the line matching the end of the script block that created the uncompiled event handler. I'm inclined to make this behaviour optional in Servo, as I believe it can help track down web compatibility problems.
Thoughts, @Ms2ger?