Bug 70991 – window.onerror doesn't work with inline (attribute) scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexey Proskuryakov]

Created attachment 112636 [details]
test case

Clicking <button onclick="for(;)">click</button> doesn't result in window.onerror being caught.

This works in Firefox, and per my understanding, catching error in inline scripts like this is the primary reason for window.onerror existence.

[Yury Semikhatsky]

Interesting, both test cases work fine in Chrome 14.0.835.202 but not in Safari 5.1.1 Of cause, uncaught errors in inline scripts should be reported as all others.

[Yury Semikhatsky]

Created attachment 113021 [details]
Patch

[Yury Semikhatsky]

The problem only reproduces on syntax errors in inline event handlers. I also added a test for exceptions(not syntax errors) in inline handlers.

[Alexey Proskuryakov]

Looks good to me. Geoff or Sam would be the best reviewers.

[Geoffrey Garen]

Comment on attachment 113021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113021&action=review

r=me

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:102
> +        reportCurrentException(exec);
>          exec->clearException();

It seems like WebCore wants an idiom where any script execution checks for and reports an exception. Long-term, it would be nice to abstract this into a common helper function / object, instead of duplicating it at each call site.

[WebKit Review Bot]

Comment on attachment 113021 [details]
Patch

Clearing flags on attachment: 113021

Committed r98885: <http://trac.webkit.org/changeset/98885>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It broke fast/js/invalid-syntax-for-function.html on all platform:
--- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/js/invalid-syntax-for-function-expected.txt 
+++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/js/invalid-syntax-for-function-actual.txt 
@@ -1,2 +1,3 @@
+CONSOLE MESSAGE: line 1: SyntaxError: Invalid character: '#'
 This test ensures we don't crash when we are given garbage for an attribute expecting a function.
 https://bugs.webkit.org/show_bug.cgi?id=19025


Could you fix it please?

[Yury Semikhatsky]

(In reply to comment #5)
> (From update of attachment 113021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113021&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/js/JSLazyEventListener.cpp:102
> > +        reportCurrentException(exec);
> >          exec->clearException();
> 
> It seems like WebCore wants an idiom where any script execution checks for and reports an exception. Long-term, it would be nice to abstract this into a common helper function / object, instead of duplicating it at each call site.

Absolutely agree, the code dealing with exceptions is scattered all over the bindings. In many cases there is only small subtle difference between JSC and v8 implementations and they could be refactored to have common logic(including general approach to exceptions reporting) in JS VM independent helper functions/objects or might be even basic classes for JS event listeners.

[Yury Semikhatsky]

(In reply to comment #8)
> It broke fast/js/invalid-syntax-for-function.html on all platform:
> --- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/js/invalid-syntax-for-function-expected.txt 
> +++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/js/invalid-syntax-for-function-actual.txt 
> @@ -1,2 +1,3 @@
> +CONSOLE MESSAGE: line 1: SyntaxError: Invalid character: '#'
>  This test ensures we don't crash when we are given garbage for an attribute expecting a function.
>  https://bugs.webkit.org/show_bug.cgi?id=19025
> 
> 
> Could you fix it please?

Done. http://trac.webkit.org/changeset/98939
Thanks for spotting this!