Bug 16829 - NPAPI: NPN_SetException() sets exception on the Global ExecState instead of local
Summary: NPAPI: NPN_SetException() sets exception on the Global ExecState instead of l...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Eric Seidel (no email)
Keywords: GoogleBug
: 19888 23201 (view as bug list)
Depends on:
Reported: 2008-01-10 15:52 PST by Jeremy Moskovich
Modified: 2009-02-05 07:06 PST (History)
7 users (show)

See Also:

First pass at set exception fix (6.83 KB, patch)
2008-02-11 14:36 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Make instances save off ExecStates, test case coming soon (27.34 KB, patch)
2008-04-08 15:25 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
NPN_SetException() implementation. (13.77 KB, patch)
2009-01-14 13:15 PST, Jeremy Moskovich
eric: review-
Details | Formatted Diff | Diff
Updated patch (13.39 KB, patch)
2009-01-14 14:02 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Updated patch (13.67 KB, patch)
2009-01-14 14:15 PST, Jeremy Moskovich
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2008-01-10 15:52:15 PST
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.
Comment 1 Eric Seidel (no email) 2008-01-10 15:58:25 PST
This is definitely a bug.  When we fix this, we'll need to add an exception-based test case to the layout tests.
Comment 2 Jeremy Moskovich 2008-01-10 16:01:52 PST
In the meantime, can anyone suggest a workaround so we can throw JS Exceptions from an NPAPI plugin in Safari?
Comment 3 Eric Seidel (no email) 2008-01-10 21:26:06 PST
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.
Comment 4 Eric Seidel (no email) 2008-02-11 14:36:28 PST
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(-)
Comment 5 Eric Seidel (no email) 2008-04-01 12:16:45 PDT
This bug blocks Google Gears support in Safari.
Comment 6 Sam Weinig 2008-04-01 15:40:18 PDT
This can probably be trivially fixed now that cache the currently executing GlobalObject for bridged js objects.
Comment 7 Eric Seidel (no email) 2008-04-08 02:05:09 PDT
Working on a real fix as we speak.
Comment 8 Eric Seidel (no email) 2008-04-08 15:25:14 PDT
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 9 Eric Seidel (no email) 2008-04-08 16:53:34 PDT
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*.
Comment 10 David Kilzer (:ddkilzer) 2008-07-20 15:27:08 PDT
Related to Bug 19888?

Comment 11 Jeremy Moskovich 2009-01-14 13:15:55 PST
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.
Comment 12 Jeremy Moskovich 2009-01-14 13:16:52 PST
The unit tests in the patch are very slightly modified versions of those posted previously by Eric.
Comment 13 Eric Seidel (no email) 2009-01-14 13:33:32 PST
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).

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.
Comment 14 Jeremy Moskovich 2009-01-14 14:02:59 PST
Created attachment 26729 [details]
Updated patch

* renamed per suggestions.
* Fixed style issues.
* removed s_exceptionEnvironment
Comment 15 Jeremy Moskovich 2009-01-14 14:15:56 PST
Created attachment 26731 [details]
Updated patch

Add asserts to check we aren't leaking exceptions.
Comment 16 Eric Seidel (no email) 2009-01-14 14:16:56 PST
Comment on attachment 26731 [details]
Updated patch

Looks great.
Comment 17 Eric Seidel (no email) 2009-01-14 14:22:50 PST
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
Comment 18 Jeremy Moskovich 2009-01-14 14:25:00 PST
*** Bug 23201 has been marked as a duplicate of this bug. ***
Comment 19 Dimitri Glazkov (Google) 2009-01-14 14:36:04 PST
Comment 20 Cameron Zwarich (cpst) 2009-02-05 07:06:35 PST
*** Bug 19888 has been marked as a duplicate of this bug. ***