Bug 39378 – [chromium] Provide a way to catch exceptions thrown while interacting with a NPObject via WebBindings methods.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Darin Fisher (:fishd, Google)]

[chromium] Provide a way to catch exceptions thrown while interacting with a NPObject via WebBindings methods.

As a consumer of WebBindings, I can invoke methods on a NPObject, but if that results in an exception being thrown, I have no way to intercept that.

[Darin Fisher (:fishd, Google)]

Created attachment 56517 [details]
v1 patch

[WebKit Review Bot]

Attachment 56517 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/V8NPUtils.cpp:161:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Created attachment 56518 [details]
v2 patch

[Nate Chapin]

Comment on attachment 56518 [details]
v2 patch

Ok.

[Darin Fisher (:fishd, Google)]

Landed at http://trac.webkit.org/changeset/59790

[Darin Fisher (:fishd, Google)]

Had to revert due to failing Chromium plugin tests (NPAPITester.NPObjectProxy)

[Darin Fisher (:fishd, Google)]

Created attachment 56565 [details]
v3 patch

This is a slight modification to the V2 patch:

1- No longer ReThrow the exception if there is not an ExceptionHandler.
This was causing problems since the next entered script context would
see the re-thrown exception.  That is clearly undesirable since the
exception could be unrelated to the next entered script context.

2- Call SetVerbose(true) if there is not a registere ExceptionHandler.
This way the exception will be logged to the JS console if it is not
being externally handled.

Interdiff:

diff -u WebCore/bindings/v8/V8NPUtils.cpp WebCore/bindings/v8/V8NPUtils.cpp
--- WebCore/bindings/v8/V8NPUtils.cpp (working copy)
+++ WebCore/bindings/v8/V8NPUtils.cpp (working copy)
@@ -152,6 +152,12 @@
     delete doomed;
 }

+ExceptionCatcher::ExceptionCatcher()
+{
+    if (!topHandler)
+        m_tryCatch.SetVerbose(true);
+}
+
 ExceptionCatcher::~ExceptionCatcher()
 {
     if (!m_tryCatch.HasCaught())
@@ -159,8 +165,6 @@

     if (topHandler)
         topHandler->handler(topHandler->data, *v8::String::Utf8Value(m_tryCatch.Exception()));
-    else
-        m_tryCatch.ReThrow();
 }

 } // namespace WebCore
diff -u WebCore/bindings/v8/V8NPUtils.h WebCore/bindings/v8/V8NPUtils.h
--- WebCore/bindings/v8/V8NPUtils.h (working copy)
+++ WebCore/bindings/v8/V8NPUtils.h (working copy)
@@ -60,6 +60,7 @@
 // current ExceptionHandler.
 class ExceptionCatcher {
 public:
+    ExceptionCatcher();
     ~ExceptionCatcher();
 private:
     v8::TryCatch m_tryCatch;

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> Created an attachment (id=56565) [details]
> v3 patch
> 
> This is a slight modification to the V2 patch:
> 
> 1- No longer ReThrow the exception if there is not an ExceptionHandler.
> This was causing problems since the next entered script context would
> see the re-thrown exception.  That is clearly undesirable since the
> exception could be unrelated to the next entered script context.

Oops, one more important note:  it appears that the JSC implementation of the _NPN_ functions also traps any exceptions and prevents them from leaking out, so this change to use v8::TryCatch is actually making the V8 <-> NPObject bindings more consistent with JSC.

[Nate Chapin]

Comment on attachment 56565 [details]
v3 patch

Looks good, assuming all the tests are now passing :)

[Darin Fisher (:fishd, Google)]

v3 patch landed as http://trac.webkit.org/changeset/59862