Bug 16829 – NPAPI: NPN_SetException() sets exception on the Global ExecState instead of local

• Component: Plug-ins
• Status: RESOLVED FIXED

[Jeremy Moskovich]

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.

[Eric Seidel (no email)]

This is definitely a bug.  When we fix this, we'll need to add an exception-based test case to the layout tests.

[Jeremy Moskovich]

In the meantime, can anyone suggest a workaround so we can throw JS Exceptions from an NPAPI plugin in Safari?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

This bug blocks Google Gears support in Safari.

[Sam Weinig]

This can probably be trivially fixed now that cache the currently executing GlobalObject for bridged js objects.

[Eric Seidel (no email)]

Working on a real fix as we speak.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[David Kilzer (:ddkilzer)]

Related to Bug 19888?

[Jeremy Moskovich]

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.

[Jeremy Moskovich]

The unit tests in the patch are very slightly modified versions of those posted previously by Eric.

[Eric Seidel (no email)]

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.

[Jeremy Moskovich]

Created attachment 26729 [details]
Updated patch

* renamed per suggestions.
* Fixed style issues.
* removed s_exceptionEnvironment

[Jeremy Moskovich]

Created attachment 26731 [details]
Updated patch

Add asserts to check we aren't leaking exceptions.

[Eric Seidel (no email)]

Comment on attachment 26731 [details]
Updated patch

Looks great.

[Eric Seidel (no email)]

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

[Jeremy Moskovich]

*** Bug 23201 has been marked as a duplicate of this bug. ***

[Dimitri Glazkov (Google)]

Awesomeness.

[Cameron Zwarich (cpst)]

*** Bug 19888 has been marked as a duplicate of this bug. ***