[webkit-reviews] review denied: [Bug 72484] [v8] Exception thrown in npObjectInvokeImpl may overwrite the exception message thrown by NPN_SetException : [Attachment 116897] Patch Updated 2

[bugzilla-daemon at webkit.org]

Nate Chapin <japhet at chromium.org> has denied Hongbo Min
<hongbo.min at intel.com>'s request for review:
Bug 72484: [v8] Exception thrown in npObjectInvokeImpl may overwrite the
exception message thrown by NPN_SetException
https://bugs.webkit.org/show_bug.cgi?id=72484

Attachment 116897: Patch Updated 2
https://bugs.webkit.org/attachment.cgi?id=116897&action=review

------- Additional Comments from Nate Chapin <japhet at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116897&action=review


> Source/WebCore/ChangeLog:21
> +	   No new tests because it depends on NPN_SetException implementation
and
> +	   JS engine used in browser. For v8 engine, it is covered by the test
in
> +	   http://codereview.chromium.org/8576001/ 

This should be testable using TestNetscapePlugin (See
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/TestNetscapePlugIn/Pl
uginObject.cpp#L1008).

> Source/WebCore/bindings/v8/V8NPObject.cpp:144
> +    if (!retval) {
> +	   // If an exception is already thrown by
invoke/invokeDefault/construct,
> +	   // native method writter is allowed to use the result as an
indicator
> +	   // to tell v8 engine that there is already a pending exception by 
> +	   // setting it as boolean type with true value. Under this case, the
> +	   // general exception will not be thrown again.
> +	   if (NPVARIANT_IS_BOOLEAN(result) && NPVARIANT_TO_BOOLEAN(result))
> +	     VOID_TO_NPVARIANT(result); // Restore back to VOID type
> +	   else
> +	     throwError("Error calling method on NPObject.",
V8Proxy::GeneralError);
> +    }

This seems dangerous, it's assigning a deep meaning to a boolean result of
true, and I'm betting it will cause regressions. There has got to be a better
way catch this case (possibly using ExceptionCatcher? I don't know much about
it, but it looks like it might be relevant)