Bug 35401 – Fix handling of errors in handling calls over bridge, where base object bridge-type does not match method bridge-type.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Gavin Barraclough]

The code assumes users will only attempt to invoke a Java method on a Java base object, etc.  This leads to unsafe casting, and crashiosity.  Should type check that the method & this object match before embarking on the method invocation, throw an exception on type mismatch.

[Gavin Barraclough]

Created attachment 49530 [details]
The patch

[Alexey Proskuryakov]

Comment on attachment 49530 [details]
The patch

> +    if (!asObject(runtimeMethod)->inherits(&CRuntimeMethod::info))
> +        return throwError(exec, TypeError, "Attempt to invoke non-C method on C object.");

The "C" hierarchy of classes is a huge misnomer, it's bindings for NPAPI plug-ins. Please don't call it "C" in text visible to Web developers.

+    const MethodList &methodList = *runtimeMethod->methods();

Misplaced &. Repeated several times in the patch.

+class ObjcRuntimeMethod : public RuntimeMethod {

The modern spelling is "ObjC". I used that for RuntimeObject, even though old code used "Objc".

+        (WebKit::):

This is just a result of a bug in the script, please delete it. Per-function comments would be even better.

+class PluginRuntimeMethod : public RuntimeMethod {

Maybe "ProxyRuntimeMethod", to match ProxyInstance and ProxyRuntimeObject?

+public:
+    PluginRuntimeMethod(ExecState* exec, const Identifier& name, Bindings::MethodList& list)
+        : RuntimeMethod(exec, name, list)
+    {
+    }
+
+    static const ClassInfo s_info;
+};

Don't you need to override classInfo() in all those classes?

Looks good, r- for lack of tests.

[Alexey Proskuryakov]

> The "C" hierarchy of classes is a huge misnomer, it's bindings for NPAPI
> plug-ins. Please don't call it "C" in text visible to Web developers.

The text should be the same as the one in WebKit, as it's the same plug-ins we are dealing with, just in-process vs. out of process.

[Gavin Barraclough]

Created attachment 49603 [details]
New awesomier patch.

[WebKit Review Bot]

Attachment 49603 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/314585

[Alexey Proskuryakov]

Comment on attachment 49603 [details]
New awesomier patch.

 __ZN7WebCore6String8fromUTF8EPKcm
+__ZTVN3JSC13RuntimeMethodE
 __ZNK3JSC8Bindings13RuntimeObject12defaultValueEPNS_9ExecStateENS_22PreferredPrimitiveTypeE

This doesn't look sorted properly.

+    virtual const ClassInfo* classInfo() const { return &info; }

In RuntimeMethod, it's s_info. Why does this work (or what is broken)?

+class CRuntimeMethod : public RuntimeMethod {

I didn't notice it last time, but we really prefer to have classes in their own files in new code.

+#import "JavaScriptCore/Error.h"
 #import "NetscapePluginHostProxy.h"
 #import "ProxyRuntimeObject.h"
 #import <WebCore/IdentifierRep.h>
 #import <WebCore/JSDOMWindow.h>
 #import <WebCore/npruntime_impl.h>
 #import <runtime/PropertyNameArray.h>
+#import "WebCore/runtime_method.h"
 
This looks like a mess. I think it should be "runtime/Error.h" and <WebCore/runtime_method.h>.

+        * java/java-and-plugins.html: Add tests for passing mismatched this objects to methdods.

The patch and ChangeLog don't include changes for expected results.

With the exception of "one class per file", the comments above clearly must be addressed before landing, r=me assuming you make the appropriate changes, and also fix Qt build.

[Gavin Barraclough]

Fixed all review issues other than the one-class-per-file one.

I strongly disagree with this change.  I believe the purpose of this rule is to make the code easier to maintain by making it simple for developers to find the definitions of classes.  However in the case of a scaffolding class only used within one .cpp I don't believe that moving this out into a header benefits anyone.  It inflates the size of the codebase, and unnecessarily hides from developers the fact that the scope of the class is constrained (the header is only included in one place, but there is no way for a future engineer coming along to know that).  I think this is a bad application of the rule, which only serves to obfuscate.

Landed in r55312