Bug 10114 – Exceptions thrown by WebKitPlugins are ignored

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dan Waylonis]

I have a WebKitPlugin.  I found that if my plugin calls [WebScriptObject throwException:@"foo"], the exception is not propagated.  The problem is that there are multiple ExecStates that are created by the interpreter when evaluating a try / catch block.  The throwException routine (and setException:) do not take this into account.

I have created a patch to fix this.  The patch includes code changes and test changes.

[Dan Waylonis]

Created attachment 9688 [details]
Patch to fix WebKitPlugin exceptions

This patch provides code to fix the bug and test cases.

[Darin Adler]

Comment on attachment 9688 [details]
Patch to fix WebKitPlugin exceptions

I want Geoff Garen or Maciej Stachowiak to review this.

[Dan Waylonis]

Hi, sounds like a good idea.  I thought about this some more and there might be a problem in a multi-view or threaded environment.  It might be better to use the pthread_getspecific() / pthread_setspecific() for the storage to the ExecStates pointer in the ObjcInstance class rather than a static variable.

[Darin Adler]

(In reply to comment #3)
> Hi, sounds like a good idea.  I thought about this some more and there might be
> a problem in a multi-view or threaded environment.  It might be better to use
> the pthread_getspecific() / pthread_setspecific() for the storage to the
> ExecStates pointer in the ObjcInstance class rather than a static variable.

I agree that per-thread would be better in principle. May not matter in practice.

[Geoffrey Garen]

Comment on attachment 9688 [details]
Patch to fix WebKitPlugin exceptions

I don't think it's right for the WebScriptObject API to be responsible for tracking the current ExecState. That's really a job for the JS engine. We attempted a similar hack to this one in JavaScriptGlue, and bad things happened (crashes, incompatibilities when the engine changed, mismatches between the stored ExecState and the real one). There's also the threading issue you mention, if we add support for ObjC plug-ins to the JS API.

The interpreter knows about the global ExecState and the most nested Context. I think a better solution would be to use that model. Giving Context a back-pointer to its sibling ExecState (ExecState already keeps a pointer to its sibling Context) is probably easiest. Then you can write code like this:

throwError(interp->context()->execState(), GeneralError, exceptionMessage);

Since Context and ExecState should probably merge in the end, the code can eventually become this:

throwError(interp->context(), GeneralError, exceptionMessage);

For now, a Context is transient and stack-allocated, so there aren't any ownership issues to worry about.

[Dan Waylonis]

Created attachment 9805 [details]
Updated patch with ggaren's suggestions

[Dan Waylonis]

I made a new patch using Geoff's suggestion to put the ExecState backpointer in the Context.  

[Geoffrey Garen]

This looks pretty good.

+    ExecState(Interpreter* interp, Context* con);

In this prototype, and others, there's no need to provide names that give no additional data. This would be better:

+    ExecState(Interpreter*, Context*);

+ExecState::ExecState(Interpreter* interp, Context* con)

In this implementation, and others, it would be clearer to use the full words "interpreter" and "context."

+    void setExecState(ExecState *es) { m_execState = es; }

Our formatting guidelines say to use "Type* " rather than "Type *", even though not all of our code follows that guideline yet. Also, it's an idiom throughout JavaScriptCore to name ExecState variables "exec", rather than "es".

 - (void)setException:(NSString *)description
 {
-    if (const RootObject *root = [self _executionContext])
-        throwError(root->interpreter()->globalExec(), GeneralError, description);
+    [[self class] throwException:description];
 }

This isn't quite right, as it won't necessarily set an exception in the target object's execution context, which is what the API promises. You really want logic that says, "Use root->interpreter()->context()->execState() if it exists, otherwise root->interpreter()->globalExec()."

+    void setExecState(ExecState *es) { m_execState = es; }
+    ExecState* execState() { return (m_execState); }

I think it's impossible to create a Context without having an ExecState available. So a clearer approach would be to require an ExecState as an argument to the Context's constructor. Then you could change this code

+          if (state) {
+            throwError(state, GeneralError, exceptionMessage);
             return YES;
+          } else
+            fprintf(stderr, "No exec state for 0x%x\n", (unsigned int)interp->context());
         }

into an assertion. Also, "state" should be "exec".

[Geoffrey Garen]

PS. "throwIfArgumentIsNotHello" is a pretty cool method name.

[Dan Waylonis]

Created attachment 9864 [details]
Patch to fix WebKitPlugin exceptions

Using Geoff's suggestions to update the patch.  I wasn't able to use the ExecState as an argument to the Context's constructor as there are times that the Context is created before the new ExecState (JSC/kjs/function.cpp: callAsFunction()) is created.

[Igor Morozov]

I got the same problem developing plugin for safari. The only difference is that I'm using NPAPI.  So if I call setexception I cannot try/catch it in Javascript. I don't get any exception at all.  Are you guys saying there is no way to throw an exception in current execution context?

[Dan Waylonis]

Comment on attachment 9864 [details]
Patch to fix WebKitPlugin exceptions

Please review.

[Darin Adler]

Comment on attachment 9864 [details]
Patch to fix WebKitPlugin exceptions

Is there a reason the ExecState function has been changed to no longer be inlined? I ask because I'm concerned about how the change might affect performance, and avoiding one extra level of function call in code that is used in only a few places seems still worthwhile.

Formatting is incorrect in ExecState.cpp: indenting is inconsistent and we normally indent the initializers rather than putting them flush left.

+    ExecState* execState() { return (m_execState); }

We don't put parentheses around return statements like these.

We'll need to do a performance test to check that this is OK.

Actually I have one big question. What guarantees that the context won't outlast the execution state? I'm worried that the code in WebScriptObject.mm might try to set the exception in a deallocated ExecState on the stack.

review- only because of that one last question. The other issues seem to be minor ones.

[Dan Waylonis]

Created attachment 10307 [details]
Patch to fix WebKitPlugin exceptions

Fix indention, formatting, parenthesis.

[Dan Waylonis]

I changed the ExecState constructor to be in the .cpp file because I couldn't get it to compile when it was inlined in the ExecState.h file.  As far as the concern about a deallocated ExecState goes, I don't know.  If I had to take a guess, it seems like ExecState and Context travel in pairs.  If you haven't already seen bugs with exceptions being thrown with stale Contexts, then you probably won't see the problem with ExecState.

[Geoffrey Garen]

Comment on attachment 9864 [details]
Patch to fix WebKitPlugin exceptions

I spoke with Darin about this.

The Context won't outlast the ExecState because both are allocated in tandem on the stack. Our current code relies on the same idiom to ensure that the ExecState won't outlast the Context.

Eventually, ExecState and Context should just merge to become the same object.

[Geoffrey Garen]

(In reply to comment #11)
> I got the same problem developing plugin for safari. The only difference is
> that I'm using NPAPI.

Igor, the NPAPI is a completely different API. With respect to the WebKit Objective-C API, no, without this patch there is no way to throw an exception in the current execution context.

[Geoffrey Garen]

I'll land this after performance testing.

[Geoffrey Garen]

Committed revision 16117.

[Timothy Hatcher]

This has landed, why is there another patch waiting to be reviewed?

[Darin Adler]

Comment on attachment 10307 [details]
Patch to fix WebKitPlugin exceptions

I'm clearing the review flag on this.