[Webkit-unassigned] [Bug 72484] [v8] Exception thrown in npObjectInvokeImpl may overwrite the exception message thrown by NPN_SetException

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72484





--- Comment #11 from Hongbo Min <hongbo.min at intel.com>  2011-11-29 22:05:12 PST ---
(In reply to comment #10)
> (From update of attachment 116897 [details])
> 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/PluginObject.cpp#L1008).

Yes, I find the test case for this patch is LayoutTests/plugins/npruntime/throw-exception.html. It doesn't break all existing tests. 

> 
> > 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)

Actually, although a deep meaning assigned to the NPVariant result, it makes sense because:
1) In case of the invoke fails, the NPVariant result will not be used as the method executing result. You may think that the result may be modified by accident even if the invoke fails, but it is indeed a bug because the result should be UNDEFINED in case of failure.
2) No impact on the current logic if no true value is set on the boolean type result. It just avoids throwing a general error if the indicator is true.

The ExceptionCatcher is useful only when there is a valid V8 context. For the NPObject which is not npScriptObjectClass, may be other NPClass, the ExceptionCatcher doesn't has much help.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.