[webkit-reviews] review granted: [Bug 8519] WebCore doesn't fire window.onerror event when uncaught JavaScript exceptions are thrown : [Attachment 76222] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 76222: Patch
https://bugs.webkit.org/attachment.cgi?id=76222&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76222&action=review

This patch is very subtle, but I think it looks good.  Sorry this took a while
to review.  A couple minor nits below that would be nice to polish up before
landing.

> LayoutTests/ChangeLog:1
> +2010-10-19  alex at webkit.org 
<alex at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>

Woah, this email address is crazy.

> WebCore/ChangeLog:28
> +	   Tests: fast/events/window-onerror1.html
> +		  fast/events/window-onerror10.html
> +		  fast/events/window-onerror11.html
> +		  fast/events/window-onerror2.html
> +		  fast/events/window-onerror3.html
> +		  fast/events/window-onerror4.html
> +		  fast/events/window-onerror5.html
> +		  fast/events/window-onerror6.html
> +		  fast/events/window-onerror7.html
> +		  fast/events/window-onerror8.html
> +		  fast/events/window-onerror9.html
> +		  http/tests/security/window-onerror-exception-in-iframe.html
> +		  userscripts/window-onerror-for-isolated-world-1.html
> +		  userscripts/window-onerror-for-isolated-world-2.html

This is an impressive set of tests.

> WebCore/bindings/js/JSErrorHandler.cpp:79
> +	   ref();

Manual ref counting is sadness.  Please use RefPtr<...> protector(this).

> WebCore/dom/ScriptExecutionContext.cpp:272
> +	       m_pendingExceptions = new Vector<OwnPtr<PendingException> >();
> +	   m_pendingExceptions->append(new PendingException(errorMessage,
lineNumber, sourceURL, callStack));

These are naked calls to new.  Can we either use adoptPtr or adoptRef?

> WebCore/dom/ScriptExecutionContext.cpp:283
> +    for (size_t i = 0; i < m_pendingExceptions->size(); i++) {

What happens if m_pendingExceptions changes during this loop?  Does
m_inDispatchErrorEvent stop that?

> WebKit/chromium/src/WebWorkerClientImpl.cpp:388
>	   thisPtr->m_scriptExecutionContext->reportException(errorMessage,
>							      lineNumber,
> -							      sourceURL);
> +							      sourceURL,
> +							      0);

You can just make this all one line.