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

[bugzilla-daemon at webkit.org]

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

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

This patch looks a bit sloppy.	Some of that might be just style errors.  The
isolated world stuff is subtle, and it feels like you're re-inventing the
wheel.	Part of the problem is that V8 doesn't really use DOMWrapperWorld,
which is something I should fix.  I'm not sure what the current state of that
is.  I'd like to see a lot more testing around isolated worlds and onerror,
since that's the toughest part.  It looks like you have one test, but you
really should test all the cases, including inline event handlers, setTimeout,
eval, etc.

> WebCore/bindings/js/JSErrorHandler.cpp:62
> +    // It is unsecure to report exception from one world to a handler from
another.

unsecure => insecure

> WebCore/bindings/v8/DOMWrapperWorld.cpp:56
> +DOMWrapperWorld* enteredContextWorld()

I don't understand this function.  Why is this different than
V8IsolatedContext::getEntered()->world() ?

> WebCore/bindings/v8/DOMWrapperWorld.cpp:59
> +    v8::Handle<v8::Context> enteredCotnext = v8::Context::GetEntered();

enteredCotnext => enteredContext

> WebCore/bindings/v8/DOMWrapperWorld.cpp:76
> +{
> +    ASSERT(scriptExecutionContext->isDocument());
> +    if (V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext)) {
> +	   v8::HandleScope handleScope;
> +	   v8::Local<v8::Context> context =
contextHandle.adjustedContext(proxy);
> +	   v8::Context::Scope contextScope(context);
> +	   return enteredContextWorld();

I don't really understand this code.  Are you truing to get the DOMWrapperWorld
for scriptExecutionContext + contextHandle ?  This seems like a very
round-about (and not necessarily correct) way of doing it.

> WebCore/bindings/v8/V8WindowErrorHandler.cpp:50
> +    // It is unsecure to report exception from one world to a handler from
another.

unsecure => insecure

> WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:77
> -	   context->reportException(errorMessage, lineNumber, sourceURL);
> +	   context->reportException(0, errorMessage, lineNumber, sourceURL, 0);


What do these zeros mean?

> WebCore/dom/ScriptExecutionContext.cpp:273
> +    if (m_pendingExceptions) {

Prefer early return.

> WebCore/dom/ScriptExecutionContext.cpp:294
> +    RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(wrapperWorld,
errorMessage, sourceURL, lineNumber));

We usually use the = form of the constructor.

> WebCore/dom/ScriptExecutionContext.h:193
> +	   struct PendingException : public Noncopyable {

class?

> WebCore/workers/WorkerContext.cpp:261
> +}

Missing a blank line after this }