Calling NPN_SetException from an NPAPI plugin doesn't work correctly (exception not visible in calling Javascript). If you pass in the plug-in object as the first parameter to NPN_SetException(), then the test in NP_jsobject.cpp#369 - _NPN_SetException() [o->_class == NPScriptObjectClass] fails and the function does nothing. If I retrieve the Window object via a call to NPN_GetValue() using NPNVWindowNPObject and pass that into NPN_SetException(), then the aforementioned test succeeds, however... _NPN_SetException() sets the exception on what appears to be the global Execstate: ExecState* exec = rootObject->globalObject()->globalExec(); But this has no effect, as the local ExecState is the one checked for an exception- see nodes.cpp#3645 ExprStatementNode::execute(). So the exception is never propagated.
This is definitely a bug. When we fix this, we'll need to add an exception-based test case to the layout tests.
In the meantime, can anyone suggest a workaround so we can throw JS Exceptions from an NPAPI plugin in Safari?
One (somewhat lame) solution would be to set a particular "__safari_exception" property on your plugin's exposed JS object, then instead of ever calling your plugin, call through a JS library and "throw" from that library any time that special property is set on your plugin object. Perhaps Anders or Geoff will have additional (better!) suggestions.
Created attachment 19080 [details] First pass at set exception fix JavaScriptCore/bindings/NP_jsobject.cpp | 19 +++++------- .../plugins/netscape-throw-exception-expected.txt | 3 ++ LayoutTests/plugins/netscape-throw-exception.html | 30 ++++++++++++++++++ .../TestNetscapePlugIn.subproj/PluginObject.cpp | 9 ++++- .../TestNetscapePlugIn.subproj/TestObject.cpp | 32 ++++++++++++++++++-- 5 files changed, 77 insertions(+), 16 deletions(-)
This bug blocks Google Gears support in Safari.
This can probably be trivially fixed now that cache the currently executing GlobalObject for bridged js objects.
Working on a real fix as we speak.
Created attachment 20413 [details] Make instances save off ExecStates, test case coming soon WebCore/bindings/objc/WebScriptObject.mm | 13 +-- WebCore/bridge/NP_jsobject.cpp | 38 +++++--- WebCore/bridge/NP_jsobject.h | 2 + WebCore/bridge/c/c_instance.cpp | 2 +- WebCore/bridge/c/c_instance.h | 4 +- WebCore/bridge/jni/jni_instance.cpp | 4 +- WebCore/bridge/jni/jni_instance.h | 4 +- WebCore/bridge/objc/objc_instance.h | 21 +++-- WebCore/bridge/objc/objc_instance.mm | 8 +- WebCore/bridge/objc/objc_runtime.mm | 4 +- WebCore/bridge/runtime.cpp | 25 +----- WebCore/bridge/runtime.h | 42 ++++++-- WebCore/bridge/runtime_method.cpp | 20 ++-- WebCore/bridge/runtime_object.cpp | 152 +++++++++++------------------- WebCore/bridge/runtime_object.h | 22 ++-- WebCore/bridge/runtime_root.h | 2 +- 16 files changed, 164 insertions(+), 199 deletions(-)
Comment on attachment 20413 [details] Make instances save off ExecStates, test case coming soon Actually, this approach won't work either, because the plugins custom objects will never be NPScriptObjects (aka JavaScriptObjects). Instead they'll be some custom NPObject "subclass" with a custom NPClass. So either I will need a way to look up the current ExecState based on NPObject*, or we'll need to just keep around a static ExecState* and set it when we enter the plugin code. We can't pass it off to NPAPI callbacks because they're on the other side of the NPAPI plugin information code. I'm interested in your thoughts. I'm leaning toward a static ExecState*.
Related to Bug 19888?
Created attachment 26726 [details] NPN_SetException() implementation. This patch mirrors the exception handling done by the obj-c bindings. The implementation ignores the NPP argument entireley which is consistent with the Mozilla implementation, see (http://mxr.mozilla.org/firefox/source/modules/plugin/base/src/ns4xPlugin.cpp line 2131). https://bugs.webkit.org/show_bug.cgi?id=19888 is also related to this issue.
The unit tests in the patch are very slightly modified versions of those posted previously by Eric.
Comment on attachment 26726 [details] NPN_SetException() implementation. I like the approach. However, there are a few things wrong here. 1. s_exceptionEnvironment seems never to be used, and can be removed. 2. There are a few code style violations (spacing around *, and { on the next line after a function declaration). http://webkit.org/coding/coding-style.html 3. I've never seen getExceptionString() style functions return a mutable reference. I've seen setExecptionString(string) used, and execptionString() style which could return a global poitner. renaming to sharedExceptionString() or globalExceptionString() might be sufficient to feel "webkitty". Parts of the NPAPI code are a sewer (from long neglect). But we still should try and update the parts we touch to modern style. An example of a part which could use update: +static bool testHasMethod(NPObject *obj, NPIdentifier name); +static bool testInvoke(NPObject *obj, NPIdentifier name, const NPVariant *args, uint32_t argCount, NPVariant *result); WK Style suggests not using named arguments in declarations, unless the name helps undestand what the argument is used for. For example, "name" is probably helps undestanding, but "obj" does not, and can be removed. r- for s_exceptionEnvironment and the various style violations.
Created attachment 26729 [details] Updated patch * renamed per suggestions. * Fixed style issues. * removed s_exceptionEnvironment
Created attachment 26731 [details] Updated patch Add asserts to check we aren't leaking exceptions.
Comment on attachment 26731 [details] Updated patch Looks great.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/plugins/netscape-throw-exception-expected.txt A LayoutTests/plugins/netscape-throw-exception.html M WebCore/ChangeLog M WebCore/bridge/NP_jsobject.cpp M WebCore/bridge/c/c_instance.cpp M WebCore/bridge/c/c_instance.h M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp M WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/TestObject.cpp Committed r39912
*** Bug 23201 has been marked as a duplicate of this bug. ***
Awesomeness.
*** Bug 19888 has been marked as a duplicate of this bug. ***