Bug 72484

Patch forthcoming

Created attachment 115502 [details] Patch

Comment on attachment 115502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115502&action=review > Source/WebCore/ChangeLog:2 > + Generic exception message thrown by npObjectInvokeImpl may overwrite ChangeLog description should start with bug title and URL. See http://www.webkit.org/coding/contributing.html#changelogs > Source/WebCore/ChangeLog:15 > + No new tests. (OOPS!) Need to provide tests or explain why none are possible. > Source/WebCore/bindings/v8/V8NPObject.cpp:136 > + // the result has a boolean type and its value should be true, so the Is this a standard technique? If so, can you point to some documentation? Or is it just a Chromium-specific work-around?

Created attachment 116645 [details] Patch Updated

Steve, thanks for your reviewing. The updated patch has been attached again. It is actually not a standard technique for solving such an issue, but it is the most reasonable and simplest solution, no need to change NPAPI interface. Suppose the situation when invoking a native method implemented in NPAPI plugin, setexception may be called to throw an exception message to indicate there is something wrong happend in the process of method execution. However, there is no way to tell V8 engine that there is already an exception message thrown. This solution is to reuse the NPVariant result that stores the result of method execution. If the method fails to execute, the NPVariant result serves as a inidcator to tell v8 engine some error occurs. It provides a chance to native method writter to throw their own exception message instead of generic message by setting NPVariant result as an boolean type and true value. Patch for http://codereview.chromium.org/8576001/ is a typical usage. The same issue also exists in JavascriptCore engine. I will fire another bug to track it. (In reply to comment #3) > (From update of attachment 115502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115502&action=review > > > Source/WebCore/ChangeLog:2 > > + Generic exception message thrown by npObjectInvokeImpl may overwrite > > ChangeLog description should start with bug title and URL. See http://www.webkit.org/coding/contributing.html#changelogs > > > Source/WebCore/ChangeLog:15 > > + No new tests. (OOPS!) > > Need to provide tests or explain why none are possible. > > > Source/WebCore/bindings/v8/V8NPObject.cpp:136 > > + // the result has a boolean type and its value should be true, so the > > Is this a standard technique? If so, can you point to some documentation? Or is it just a Chromium-specific work-around?

Comment on attachment 116645 [details] Patch Updated View in context: https://bugs.webkit.org/attachment.cgi?id=116645&action=review > Source/WebCore/bindings/v8/V8NPObject.cpp:135 > + // If an exception is already thrown by invoke/invokeDefault/construct, I think this could be worded more clearly - 'A return value of true is used to indicate that an exception has been thrown.' > Source/WebCore/bindings/v8/V8NPObject.cpp:139 > + if (NPVARIANT_IS_BOOLEAN(result) && NPVARIANT_TO_BOOLEAN(result)) Presumably we can't test directly for an exception having been thrown? If not, this approach seems reasonable to me, but you should get a formal review from whoever owns this code.

Great thank to Steve. Alert & Dimitri, Could you please take a time to review this patch? Thanks

Failed to upload the new patch. The Patch Updated is still valid.

Created attachment 116897 [details] Patch Updated 2

Comment on attachment 116897 [details] Patch Updated 2 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). > 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)

(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.

Any comment? Nate.

(In reply to comment #12) > Any comment? Nate. What do Safari, Firefox, and Opera do in this case? We should try to match them if at all possible.

For Safari, Firefox and Opera, they are both not using v8 as javascript engine. In this sense, this patch serves as a chromium-specific bug-fixing. It just provides a chance to solve such an issue but depending on the implementation in browser-side. In order to make chromium throwing the correct message set by NPN_SetException, chromium itself needs to apply another patch as described at http://codereview.chromium.org/8576001/ I had a try on Safari/Firefox/Opera, they also can't throw the expected message set by NPN_SetException. As far as I concerned, this patch is the simplest solution for solving this issue for chromium/v8. I have also tried other solution, but will involve more changes even if NPAPI interface spec. (In reply to comment #13) > (In reply to comment #12) > > Any comment? Nate. > > What do Safari, Firefox, and Opera do in this case? We should try to match them if at all possible.

(In reply to comment #14) > For Safari, Firefox and Opera, they are both not using v8 as javascript engine. In this sense, this patch serves as a chromium-specific bug-fixing. It just provides a chance to solve such an issue but depending on the implementation in browser-side. > > In order to make chromium throwing the correct message set by NPN_SetException, chromium itself needs to apply another patch as described at http://codereview.chromium.org/8576001/ > > I had a try on Safari/Firefox/Opera, they also can't throw the expected message set by NPN_SetException. > > As far as I concerned, this patch is the simplest solution for solving this issue for chromium/v8. I have also tried other solution, but will involve more changes even if NPAPI interface spec. If the v8/chromium behavior is currently the same as Safari, Firefox, and Opera, then we shouldn't change our behavior. Plugin authors expect chromium's NPAPI implementation to match the other NPAPI implementors as much as possible. If you feel there is a bug in the NPAPI specification, you're welcome to talk to plugin-futures (https://mail.mozilla.org/listinfo/plugin-futures) about it. Without plugin-futures support for changing the behavior, however, I don't think this patch is a good idea.

(In reply to comment #15) > (In reply to comment #14) > > For Safari, Firefox and Opera, they are both not using v8 as javascript engine. In this sense, this patch serves as a chromium-specific bug-fixing. It just provides a chance to solve such an issue but depending on the implementation in browser-side. > > > > In order to make chromium throwing the correct message set by NPN_SetException, chromium itself needs to apply another patch as described at http://codereview.chromium.org/8576001/ > > > > I had a try on Safari/Firefox/Opera, they also can't throw the expected message set by NPN_SetException. > > > > As far as I concerned, this patch is the simplest solution for solving this issue for chromium/v8. I have also tried other solution, but will involve more changes even if NPAPI interface spec. > > If the v8/chromium behavior is currently the same as Safari, Firefox, and Opera, then we shouldn't change our behavior. Plugin authors expect chromium's NPAPI implementation to match the other NPAPI implementors as much as possible. From this aspect, it makes sense that we need to match the same behavior between Safari, Firefox and chromium, etc, although it is indeed a bug. > > If you feel there is a bug in the NPAPI specification, you're welcome to talk to plugin-futures (https://mail.mozilla.org/listinfo/plugin-futures) about it. Without plugin-futures support for changing the behavior, however, I don't think this patch is a good idea.

V8 is gone from WebKit.