Plugins should not be allowed to override standard properties/attributes in non-standard worlds. In radar as <rdar://problem/10598594>
(In reply to comment #0) >In radar as <rdar://problem/10598594> Correction - <rdar://problem/11975252>
Created attachment 154996 [details] Patch v1 - Fix + regression test
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.
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.
(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 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
(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.
> 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.
(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.
Created attachment 155022 [details] Patch v2 - Now with uglier test but prettier test results
(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.
(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?
Created attachment 155038 [details] Patch v3 - Don't touch the bindings script. Much better!
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.
Created attachment 155043 [details] Patch v4 - Now with more templates
(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.
Ok. I've said my piece. Thanks for listening.
(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 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
(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 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
(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...
Actually, I am seeing a build failure locally with a clean build... figuring it out now.
Duh, templates need to be in header... fixing...
Created attachment 155055 [details] Patch v4a - Uploading just for EWS
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
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
(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.
(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.
> 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
Still waiting on an all-clear from qt then I'll land.
(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 :)
Sorry, I think our comments collided in bugzilla.
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
Final patch (except for chromium expectations) has passed ews, landing soon!
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
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
Landed in http://trac.webkit.org/changeset/123936