Bug 92519 - Plugins should not be allowed to override standard properties/attributes in non-standard worlds
Summary: Plugins should not be allowed to override standard properties/attributes in n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-07-27 11:09 PDT by Brady Eidson
Modified: 2012-07-27 16:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 - Fix + regression test (22.40 KB, patch)
2012-07-27 11:30 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Now with uglier test but prettier test results (22.95 KB, patch)
2012-07-27 12:29 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v3 - Don't touch the bindings script. Much better! (22.38 KB, patch)
2012-07-27 13:25 PDT, Brady Eidson
andersca: review-
Details | Formatted Diff | Diff
Patch v4 - Now with more templates (24.65 KB, patch)
2012-07-27 13:51 PDT, Brady Eidson
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch v4a - Uploading just for EWS (23.47 KB, patch)
2012-07-27 14:49 PDT, Brady Eidson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (1.15 MB, application/zip)
2012-07-27 15:50 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-05 (941.40 KB, application/zip)
2012-07-27 16:43 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-07-27 11:09:39 PDT
Plugins should not be allowed to override standard properties/attributes in non-standard worlds.

In radar as <rdar://problem/10598594>
Comment 1 Brady Eidson 2012-07-27 11:11:50 PDT
(In reply to comment #0)
>In radar as <rdar://problem/10598594>

Correction - <rdar://problem/11975252>
Comment 2 Brady Eidson 2012-07-27 11:30:30 PDT
Created attachment 154996 [details]
Patch v1 - Fix + regression test
Comment 3 Sam Weinig 2012-07-27 11:33:42 PDT
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.
Comment 4 Adam Barth 2012-07-27 12:09:09 PDT
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.
Comment 5 Brady Eidson 2012-07-27 12:14:30 PDT
(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.
Comment 6 Anders Carlsson 2012-07-27 12:19:03 PDT
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
Comment 7 Anders Carlsson 2012-07-27 12:19:59 PDT
(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.
Comment 8 Adam Barth 2012-07-27 12:20:08 PDT
> 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.
Comment 9 Brady Eidson 2012-07-27 12:28:24 PDT
(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.
Comment 10 Brady Eidson 2012-07-27 12:29:03 PDT
Created attachment 155022 [details]
Patch v2 - Now with uglier test but prettier test results
Comment 11 Brady Eidson 2012-07-27 12:31:39 PDT
(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.
Comment 12 Brady Eidson 2012-07-27 12:35:11 PDT
(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?
Comment 13 Brady Eidson 2012-07-27 13:25:13 PDT
Created attachment 155038 [details]
Patch v3 - Don't touch the bindings script.  Much better!
Comment 14 Anders Carlsson 2012-07-27 13:28:40 PDT
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.
Comment 15 Brady Eidson 2012-07-27 13:51:52 PDT
Created attachment 155043 [details]
Patch v4 - Now with more templates
Comment 16 Brady Eidson 2012-07-27 13:52:27 PDT
(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.
Comment 17 Adam Barth 2012-07-27 13:56:04 PDT
Ok.  I've said my piece.  Thanks for listening.
Comment 18 Brady Eidson 2012-07-27 13:57:59 PDT
(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.  :)
Comment 19 Build Bot 2012-07-27 14:06:54 PDT
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
Comment 20 Brady Eidson 2012-07-27 14:20:31 PDT
(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.
Comment 21 Early Warning System Bot 2012-07-27 14:31:46 PDT
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
Comment 22 Brady Eidson 2012-07-27 14:38:00 PDT
(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...
Comment 23 Brady Eidson 2012-07-27 14:40:51 PDT
Actually, I am seeing a build failure locally with a clean build...  figuring it out now.
Comment 24 Brady Eidson 2012-07-27 14:44:21 PDT
Duh, templates need to be in header... fixing...
Comment 25 Brady Eidson 2012-07-27 14:49:29 PDT
Created attachment 155055 [details]
Patch v4a - Uploading just for EWS
Comment 26 WebKit Review Bot 2012-07-27 15:50:37 PDT
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
Comment 27 WebKit Review Bot 2012-07-27 15:50:42 PDT
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
Comment 28 Brady Eidson 2012-07-27 15:53:47 PDT
(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.
Comment 29 Brady Eidson 2012-07-27 15:55:46 PDT
(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.
Comment 30 Adam Barth 2012-07-27 15:56:22 PDT
> 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
Comment 31 Brady Eidson 2012-07-27 16:02:53 PDT
Still waiting on an all-clear from qt then I'll land.
Comment 32 Brady Eidson 2012-07-27 16:04:43 PDT
(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  :)
Comment 33 Adam Barth 2012-07-27 16:14:35 PDT
Sorry, I think our comments collided in bugzilla.
Comment 34 Early Warning System Bot 2012-07-27 16:28:55 PDT
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
Comment 35 Brady Eidson 2012-07-27 16:37:20 PDT
Final patch (except for chromium expectations) has passed ews, landing soon!
Comment 36 WebKit Review Bot 2012-07-27 16:43:15 PDT
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
Comment 37 WebKit Review Bot 2012-07-27 16:43:21 PDT
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
Comment 38 Brady Eidson 2012-07-27 16:44:53 PDT
Landed in http://trac.webkit.org/changeset/123936