Bug 92519 – Plugins should not be allowed to override standard properties/attributes in non-standard worlds

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Brady Eidson]

Plugins should not be allowed to override standard properties/attributes in non-standard worlds.

In radar as <rdar://problem/10598594>

[Brady Eidson]

(In reply to comment #0)
>In radar as <rdar://problem/10598594>

Correction - <rdar://problem/11975252>

[Brady Eidson]

Created attachment 154996 [details]
Patch v1 - Fix + regression test

[Sam Weinig]

Comment on attachment 154996 [details]
Patch v1 - Fix + regression test

View in context: https://bugs.webkit.org/attachment.cgi?id=154996&action=review

> LayoutTests/plugins/npruntime/overrides-all-properties.html:12
> +		log("This test can only be run under WebKit's testRunner environment");
> +		return;

Tabs :(.  There are more in here as well.

> LayoutTests/plugins/npruntime/overrides-all-properties.html:35
> +		\
> +		log('madeUpProperty' in pluginEmbed); \
> +		log(typeof pluginEmbed.madeUpProperty); \
> +		log(pluginEmbed.madeUpProperty); \
> +		log('getAttribute' in pluginEmbed); \
> +		log(typeof pluginEmbed.getAttribute); \
> +		log(pluginEmbed.getAttribute); \
> +		\
> +		log(Object.getOwnPropertyDescriptor(pluginEmbed, 'madeUpProperty')); \
> +		log(Object.getOwnPropertyDescriptor(pluginEmbed, 'getAttribute')); \
> +		\

This could be made much clearer, but having the logging say PASS / FAIL, or at least what is expected.

[Adam Barth]

It looks like you're trying to create something like an XPCNativeWrapper in this patch.  I'm not entirely sure what your goals are for doing that, but one of the main benefits of the isolated worlds design is to avoid the need for an XPCNativeWrapper-like mechanism.

[Brady Eidson]

(In reply to comment #4)
> It looks like you're trying to create something like an XPCNativeWrapper in this patch.  I'm not entirely sure what your goals are for doing that, but one of the main benefits of the isolated worlds design is to avoid the need for an XPCNativeWrapper-like mechanism.

I don't know what an XPCNativeWrapper is, so I can't say whether or not that is my goal.

[Anders Carlsson]

Comment on attachment 154996 [details]
Patch v1 - Fix + regression test

View in context: https://bugs.webkit.org/attachment.cgi?id=154996&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:422
>  sub GenerateGetOwnPropertySlotBody
>          push(@getOwnPropertySlotImpl, "    ${namespaceMaybe}JSValue proto = thisObject->prototype();\n");
>          push(@getOwnPropertySlotImpl, "    if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(asObject(proto))->hasProperty(exec, propertyName))\n");
>          push(@getOwnPropertySlotImpl, "        return false;\n\n");
> +    } else {
> +        if ($dataNode->extendedAttributes->{"PluginOwner"}) {
> +            push(@getOwnPropertySlotImpl, "    if (!thisObject->globalObject()->world()->isNormal()) {\n");
> +            push(@getOwnPropertySlotImpl, "        if (${namespaceMaybe}getStaticValueSlot<$className, Base>(exec, " . hashTableAccessor($dataNode->extendedAttributes->{"JSNoStaticTables"}, $className) . ", thisObject, propertyName, slot))\n");
> +            push(@getOwnPropertySlotImpl, "            return true;\n\n");
> +            push(@getOwnPropertySlotImpl, "        ${namespaceMaybe}JSValue proto = thisObject->prototype();\n");
> +            push(@getOwnPropertySlotImpl, "        if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(asObject(proto))->hasProperty(exec, propertyName))\n");
> +            push(@getOwnPropertySlotImpl, "            return false;\n\n");
> +            push(@getOwnPropertySlotImpl, "        return thisObject->getOwnPropertySlotDelegate(exec, propertyName, slot);\n");
> +            push(@getOwnPropertySlotImpl, "    }\n\n");
> +        }
>      }
>  
>      my $manualLookupGetterGeneration = sub {

I don't think any of this is needed - you should be able to do all of this in the functions in JSPluginElementFunctions.cpp

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 154996 [details])
> 
> This could be made much clearer, but having the logging say PASS / FAIL, or at least what is expected.

I fully agree - it's not easy for someone to see what this test is supposed to do, or what the expected result is.

[Adam Barth]

> I don't know what an XPCNativeWrapper is, so I can't say whether or not that is my goal.

https://developer.mozilla.org/en/XPCNativeWrapper

This mechanism is a bit different in the sense that you're not trying to "see through" modifications to the JavaScript wrapper.  The sense in which it's similar is that you're trying to prevent the modifications so that the JavaScript wrapper is a "faithful" representation of the underlying object.

Unfortunately, that tends to be only the tip of the iceberg.  Without understanding what you're trying to accomplish with this patch it's hard to comment on whether the patch succeed in achieving those goals.  I can tell you, however, that other folks who have started down a similar path tend to wind up unhappy with where the path leads.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 154996 [details] [details])
> > 
> > This could be made much clearer, but having the logging say PASS / FAIL, or at least what is expected.
> 
> I fully agree - it's not easy for someone to see what this test is supposed to do, or what the expected result is.

Since the test cannot run in the browser and could only run under our testing infrastructure, I figured looking at a diff of expected results would clearly show any failures.

Since both of you strongly agree, I've PASS/FAILed it.  This makes the test itself uglier, but results in a series of "PASS"es in the expected file.

[Brady Eidson]

Created attachment 155022 [details]
Patch v2 - Now with uglier test but prettier test results

[Brady Eidson]

(In reply to comment #8)
> > I don't know what an XPCNativeWrapper is, so I can't say whether or not that is my goal.
> 
> https://developer.mozilla.org/en/XPCNativeWrapper
> 
> This mechanism is a bit different in the sense that you're not trying to "see through" modifications to the JavaScript wrapper.  The sense in which it's similar is that you're trying to prevent the modifications so that the JavaScript wrapper is a "faithful" representation of the underlying object.
> 
> Unfortunately, that tends to be only the tip of the iceberg.  Without understanding what you're trying to accomplish with this patch it's hard to comment on whether the patch succeed in achieving those goals.  I can tell you, however, that other folks who have started down a similar path tend to wind up unhappy with where the path leads.

I looked at the first few paragraphs of that page and then decided TLDR;

Our goal is to prevent needing access to the plugin script object (and therefore creation of the plugin script object) when accessing built-in properties in non-standard worlds.

This patch seems to do that successfully.

[Brady Eidson]

(In reply to comment #6)
> (From update of attachment 154996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154996&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:422
> >  sub GenerateGetOwnPropertySlotBody
> >          push(@getOwnPropertySlotImpl, "    ${namespaceMaybe}JSValue proto = thisObject->prototype();\n");
> >          push(@getOwnPropertySlotImpl, "    if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(asObject(proto))->hasProperty(exec, propertyName))\n");
> >          push(@getOwnPropertySlotImpl, "        return false;\n\n");
> > +    } else {
> > +        if ($dataNode->extendedAttributes->{"PluginOwner"}) {
> > +            push(@getOwnPropertySlotImpl, "    if (!thisObject->globalObject()->world()->isNormal()) {\n");
> > +            push(@getOwnPropertySlotImpl, "        if (${namespaceMaybe}getStaticValueSlot<$className, Base>(exec, " . hashTableAccessor($dataNode->extendedAttributes->{"JSNoStaticTables"}, $className) . ", thisObject, propertyName, slot))\n");
> > +            push(@getOwnPropertySlotImpl, "            return true;\n\n");
> > +            push(@getOwnPropertySlotImpl, "        ${namespaceMaybe}JSValue proto = thisObject->prototype();\n");
> > +            push(@getOwnPropertySlotImpl, "        if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(asObject(proto))->hasProperty(exec, propertyName))\n");
> > +            push(@getOwnPropertySlotImpl, "            return false;\n\n");
> > +            push(@getOwnPropertySlotImpl, "        return thisObject->getOwnPropertySlotDelegate(exec, propertyName, slot);\n");
> > +            push(@getOwnPropertySlotImpl, "    }\n\n");
> > +        }
> >      }
> >  
> >      my $manualLookupGetterGeneration = sub {
> 
> I don't think any of this is needed - you should be able to do all of this in the functions in JSPluginElementFunctions.cpp

Sorry I didn't seen this comment before v2 because there was a lot of noise.

When I first tried to do this in JSPluginElementFunctions.cpp I ran into reentrancy - JS kept calling back in to the getter delegate.  We had to short-circuit JS before it called the delegate.

Any ideas on how to avoid that?

[Brady Eidson]

Created attachment 155038 [details]
Patch v3 - Don't touch the bindings script.  Much better!

[Anders Carlsson]

Comment on attachment 155038 [details]
Patch v3 - Don't touch the bindings script.  Much better!

View in context: https://bugs.webkit.org/attachment.cgi?id=155038&action=review

> Source/WebCore/bindings/js/JSHTMLAppletElementCustom.cpp:46
> +    if (!globalObject()->world()->isNormal()) {
> +        if (getStaticValueSlot<JSHTMLAppletElement, Base>(exec, s_info.staticPropHashTable, this, propertyName, slot))
> +            return true;
> +
> +        JSValue proto = prototype();
> +        if (proto.isObject() && jsCast<JSObject*>(asObject(proto))->hasProperty(exec, propertyName))
> +            return false;
> +    }
> +

I think you should factor this code out into a function template and put it in JSPluginElementFunctions.h

> Tools/DumpRenderTree/TestNetscapePlugIn/Tests/PluginScriptableObjectOverridesAllProperties.cpp:60
> +            int stringLength = strlen(propertyString) + strlen(message) + 1;

I'd call this bufferLenght instead of stringLength.

[Brady Eidson]

Created attachment 155043 [details]
Patch v4 - Now with more templates

[Brady Eidson]

(In reply to comment #14)
Tools/DumpRenderTree/TestNetscapePlugIn/Tests/PluginScriptableObjectOverridesAllProperties.cpp:60
> > +            int stringLength = strlen(propertyString) + strlen(message) + 1;
> 
> I'd call this bufferLenght instead of stringLength.

I accidentally ignored this comment in patch v4, will address before landing.

[Adam Barth]

Ok.  I've said my piece.  Thanks for listening.

[Brady Eidson]

(In reply to comment #17)
> Ok.  I've said my piece.  Thanks for listening.

I hope I didn't give the impression that I was blowing you off.  I appreciate the feedback.  I just don't think our goal here was anywhere near as grand as you were suspecting.  :)

[Build Bot]

Comment on attachment 155043 [details]
Patch v4 - Now with more templates

Attachment 155043 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13373644

[Brady Eidson]

(In reply to comment #19)
> (From update of attachment 155043 [details])
> Attachment 155043 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13373644

Dunno what this is about - Built fine locally.

Updating sources and building before landing.

[Early Warning System Bot]

Comment on attachment 155043 [details]
Patch v4 - Now with more templates

Attachment 155043 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13370722

[Brady Eidson]

(In reply to comment #21)
> (From update of attachment 155043 [details])
> Attachment 155043 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/13370722

The function Qt is claiming is undefined is quite clearly defined in JSPluginElementFunctions.cpp

If that file wasn't included in the Qt build this already wouldn't have build even before this change.  Confused...

[Brady Eidson]

Actually, I am seeing a build failure locally with a clean build...  figuring it out now.

[Brady Eidson]

Duh, templates need to be in header... fixing...

[Brady Eidson]

Created attachment 155055 [details]
Patch v4a - Uploading just for EWS

[WebKit Review Bot]

Comment on attachment 155043 [details]
Patch v4 - Now with more templates

Attachment 155043 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13377665

New failing tests:
plugins/npruntime/overrides-all-properties.html

[WebKit Review Bot]

Created attachment 155076 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brady Eidson]

(In reply to comment #26)
> (From update of attachment 155043 [details])
> Attachment 155043 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13377665
> 
> New failing tests:
> plugins/npruntime/overrides-all-properties.html

I don't know what I'm supposed to do with this.  I clicked the output link - http://queues.webkit.org/results/13377665 - and searched for my new test and it didn't show up anywhere.

[Brady Eidson]

(In reply to comment #27)
> Created an attachment (id=155076) [details]
> Archive of layout-test-results from gce-cr-linux-05
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Based on this output it's clear this is the V8 bindings behaving differently.  I'll skip on chromium.

[Adam Barth]

> I don't know what I'm supposed to do with this.

Presumably you want to mark this test as expected to fail in http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations

[Brady Eidson]

Still waiting on an all-clear from qt then I'll land.

[Brady Eidson]

(In reply to comment #30)
> > I don't know what I'm supposed to do with this.
> 
> Presumably you want to mark this test as expected to fail in http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations

That's what I said...

(In reply to comment #29)
> (In reply to comment #27)
> > Created an attachment (id=155076) [details] [details]
> > Archive of layout-test-results from gce-cr-linux-05
> > 
> > The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> > Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
> 
> Based on this output it's clear this is the V8 bindings behaving differently.  I'll skip on chromium.

...here  :)

[Adam Barth]

Sorry, I think our comments collided in bugzilla.

[Early Warning System Bot]

Comment on attachment 155043 [details]
Patch v4 - Now with more templates

Attachment 155043 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13372749

[Brady Eidson]

Final patch (except for chromium expectations) has passed ews, landing soon!

[WebKit Review Bot]

Comment on attachment 155055 [details]
Patch v4a - Uploading just for EWS

Attachment 155055 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13372756

New failing tests:
plugins/npruntime/overrides-all-properties.html

[WebKit Review Bot]

Created attachment 155089 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/123936