[webkit-reviews] review denied: [Bug 8519] WebCore doesn't fire window.onerror event when uncaught JavaScript exceptions are thrown : [Attachment 65109] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 9 18:22:40 PDT 2010
Sam Weinig <sam at webkit.org> has denied Yury Semikhatsky <yurys at chromium.org>'s
request for review:
Bug 8519: WebCore doesn't fire window.onerror event when uncaught JavaScript
exceptions are thrown
https://bugs.webkit.org/show_bug.cgi?id=8519
Attachment 65109: Patch
https://bugs.webkit.org/attachment.cgi?id=65109&action=review
------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=65109&action=prettypatch
> LayoutTests/fast/events/window-onerror2.html:19
> +window.onerror = function(msg, url, line)
> +{
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> +}
This would read more clearly if it returned false, instead of relying on the
implicit return value of undefined being interpreted as false.
> LayoutTests/fast/events/window-onerror2.html:30
> +function delayedThrowException()
> +{
> + throw "Exception in setTimeout";
> +}
> +setTimeout(delayedThrowException, 0);
> +
> +setTimeout(function() {
> + if (window.layoutTestController)
> + layoutTestController.notifyDone();
> +}, 0);
These racing setTimeouts seem unnecessarily complex/unreliable. Why not just
have onerror keep a count, and notifyDone when it hits 3.
> LayoutTests/fast/events/window-onerror3.html:22
> +
> +function log(msg) {
> + document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> + window.onerror = function (error, url, line) {
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Error caught successfully: " + error + "\nFile: " + url +
"\nLine: " + line)
> + };
> + unknownObject.unknownProperty++;
> +}
> +</script>
> +<body onload="test1();">
> +<p>You should see a message if window.onerror is working properly for this
test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug 8519</a>.</p>
> +<hr>
> +<div id='console'></div>
> +</body>
It seems like there should be no console message here since the onerror handler
is returning false (meaning handled). Is this a bug? You also should
explicitly return false.
> LayoutTests/fast/events/window-onerror4.html:22
> +
> +function log(msg) {
> + document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> + window.onerror = function (error, url, line) {
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Error caught successfully: " + error + "\nFile: " + url +
"\nLine: " + line)
> + };
> + eval("1=2");
> +}
> +</script>
> +<body onload="test1();">
> +<p>You should see a log record if window.onerror is working properly for
this test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug
8519</a>.</p>
> +<hr>
> +<div id='console'></div>
> +</body>
Same question as above?
> LayoutTests/fast/events/window-onerror5.html:17
> +function log(msg) {
> + document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> + window.onerror = function (error, url, line) {
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Error caught successfully: " + error + "\nFile: " + url +
"\nLine: " + line)
> + };
> + eval("a(");
> +}
> +</script>
Here too.
> LayoutTests/fast/events/window-onerror6.html:23
> + document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +window.onerror = function(msg, url, line)
> +{
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> +}
> +</script>
> +<script>
> +
> +a) // syntax error
> +
> +</script>
Same question.
> LayoutTests/fast/events/window-onerror7.html:24
> +
> +function log(msg) {
> + document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +window.onerror = function(msg, url, line)
> +{
> + url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> + log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> + throw "Nested error";
> +}
> +
> +throw 2010;
> +
> +</script>
> +</body>
> +</html>
According to HTML5:
"Any uncaught exceptions thrown or errors caused by this function may be
reported to the user immediately after the error that the function was called
for; the report an error algorithm must not be used to handle exceptions thrown
or errors caused by this function."
It seems like the nested error is being reported first which seems incorrect.
> WebCore/dom/Document.cpp:4450
> + // Invoke window.onerror.
> + if (invokeOnErrorHandler(domWindow->onerror(), errorMessage,
lineNumber, sourceURL))
> + return;
> + }
You should remove the comment. The code is clear enough not to need one.
> WebCore/dom/ScriptExecutionContext.cpp:275
> +bool ScriptExecutionContext::invokeOnErrorHandler(EventListener* onerror,
const String& errorMessage, int lineNumber, const String& sourceURL)
> +{
> + if (!onerror)
> + return false;
> + if (m_inErrorHandler)
> + return false;
> + m_inErrorHandler = true;
> + RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(errorMessage,
sourceURL, lineNumber));
> + onerror->handleEvent(this, errorEvent.get());
> + m_inErrorHandler = false;
> + return errorEvent->defaultPrevented();
> +}
There are a few problems with this code.
1) It doesn't dispatch the event to listeners added via addEventListener()
2) It doesn't set the properties on the event like "target", "phase" and
"currentTarget". Even though the event object is not available as an argument,
it will be both in scope and available on the global object.
There is an easy solution though, the standard way to due this is to call
dispatchEvent on the EventTarget. Please make this change and add tests for
each of these bugs.
> WebCore/dom/ScriptExecutionContext.h:98
> + bool invokeOnErrorHandler(EventListener*, const String&
errorMessage, int lineNumber, const String& sourceURL);
Error handler is too generic a name. How about we call this dispatchErrorEvent.
> WebCore/dom/ScriptExecutionContext.h:188
> + bool m_inErrorHandler;
This should probably be renamed to m_inDispatchErrorEvent. Thats what we do for
other recursion guards in WebCore.
r- due to the correctness problems but this is a very promising patch.
More information about the webkit-reviews
mailing list