* SUMMARY Native Bindings Descriptors are Incomplete. We are not returning a getter/function and the configurable property looks incorrect. According to WebIDL: http://www.w3.org/TR/WebIDL/#es-attributes > For each attribute defined on the interface, there must exist a corresponding property. If the attribute was declared > with the [Unforgeable] extended attribute, then the property exists on every object that implements the interface. > Otherwise, it exists on the interface’s interface prototype object. > > The characteristics of these properties are as follows: > > - The name of the property is the identifier of the attribute. > - The property has attributes { [[Get]]: G, [[Set]]: S, [[Enumerable]]: true, [[Configurable]]: configurable }, ... > - G is the attribute getter, defined below; and > - S is the attribute setter, also defined below. > - The attribute getter is a Function object ... > - The attribute setter is undefined if the attribute is declared readonly and has neither a [PutForwards] nor > a [Replaceable] extended attribute declared on it. Otherwise, it is a Function object ... For example, in MouseEvent: (simplified) interface MouseEvent : UIEvent { readonly attribute boolean altKey; }; WebKit gives us a descriptor with empty get/set and configurable "false" but should be true: webkit-console> Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey"); { configurable: false enumerable: true get: undefined set: undefined } I expect something more like Firefox: firefox-console> Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey"); Object { configurable: true, enumerable: true, get: altKey(), set: undefined } Note that Chrome's bindings are on the instance, not prototype, and so don't seem to match the spec. chrome-console> Object.getOwnPropertyDescriptor(x, "altKey") Object {value: false, writable: true, enumerable: true, configurable: true}
<rdar://problem/19506502>
JSObject::getOwnPropertyDescriptor calls for CustomAccessors: void PropertyDescriptor::setCustomDescriptor(unsigned attributes) { m_attributes = attributes | Accessor | CustomAccessor; m_attributes &= ~ReadOnly; m_seenAttributes = EnumerablePresent | ConfigurablePresent; setGetter(jsUndefined()); setSetter(jsUndefined()); m_value = JSValue(); } As far as I can tell, only our bindings are CustomAccessors.
Created attachment 246543 [details] [PATCH] Approach #1 No tests written yet, but this produces expected results for me: js> Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey").get === Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey").get true js> mouseEvent.altKey false js> getter = Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey").get function altKey() { [native code] } js> getter.call(mouseEvent) false Is this approach reasonable? Things to look into next: - move JSValues off of CustomGetterSetter and into a WeakGCMap - tests (like what I have above)
Created attachment 246565 [details] [PATCH] Proposed Fix
Attachment 246565 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246565 [details] [PATCH] Proposed Fix Attachment 246565 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5357833683992576 New failing tests: inspector-protocol/runtime/getProperties.html
Created attachment 246569 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
The failing test is: inspector-protocol/runtime/getProperties.html --- layout-test-results/inspector-protocol/runtime/getProperties-expected.txt +++ layout-test-results/inspector-protocol/runtime/getProperties-actual.txt @@ -9,8 +9,8 @@ length number 3 Properties of Bound function __proto__ function undefined - arguments - caller + arguments object undefined + caller object undefined length number 0 name string Number This is because: js> Number.bind({}, 5).arguments TypeError: Type Error Not a very descriptive error! However, this is something we can probably handle a bit better in InjectedScriptSource. r- for me to clean this up.
> This is because: > > js> Number.bind({}, 5).arguments > TypeError: Type Error Hmm: js> Object.getOwnPropertyDescriptor(function(){}, "arguments") < {value: null, writable: false, enumerable: false, configurable: false} js> Object.getOwnPropertyDescriptor(function(){}.bind(this), "arguments") < {get: function, set: function, enumerable: false, configurable: false} I'm going to file a separate issue for that. This duel identity seems weird. Also, I think this is another property expected to move to the prototype.
Created attachment 246689 [details] [PATCH] Proposed Fix
Comment on attachment 246689 [details] [PATCH] Proposed Fix I'm going to r- this. - Patch is over a month old and probably stale - I've gotten a better understanding of bindings now, and I don't think the JSAnchoredFunction concept is needed. The bindings did not actually care what the prototype object was, the generated code just ignores the argument... - Web Inspector worked around this issue in all cases where this was previously an issue. It may happen again some time in the future, but for now we don't actually need the getter / setter function to be called later. - After discussion with Sam, we should probably look at removing the CustomAccessor special case for Bindings and making it work like a regular Accessor.
Created attachment 255043 [details] [Patch] New proposed fix Updated the patch to apply to trunk. Also does not set a setter or getter if none is present, as that would cause a call of that getter/setter to crash the jit. Also allows for configurable to still be set to false. As it turns out, the generated code for the DOM bindings it using it's parameters so they must be passed to the getter/setter.
Created attachment 255122 [details] [Patch] New proposed fix fixed build problem and added test expectations.
Comment on attachment 255122 [details] [Patch] New proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=255122&action=review Some drive by comments. > LayoutTests/js/script-tests/native-bindings-descriptors.js:8 > +var node_type = Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType'); > +console.log("NodeType: ", node_type); > +console.log("NodeType.get: ", node_type.get); > + > +shouldBe("node_type.get != undefined", "true"); > +shouldBe("node_type.get.call(document.body)", "1"); > +shouldBe("node_type.enumerable", "true"); This is a very limp test. The console.log's don't appear to output useful data, only the first string parameter. At the very least we should compare that getting the getter multiple times is the same getter, which is the purpose of the WeakGCMap on VM: var getter1 = Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType').get; var getter2 = Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType').get; shouldBeTrue("getter1 === getter2"); I'd recommend adding some of the tests from my original, obsoleted patch. It tested a number of different bindings generated in different ways, and properties of the resulting functions (toString, length, identity) not covered here. fast/dom/document-property-descriptors.html / fast/dom/event-property-descriptors.html https://bugs.webkit.org/attachment.cgi?id=246689&action=review Also, since this test references DOM objects (Node) I don't think it should live in LayoutTests/js. I believe LayoutTests/js/script-tests are either performance / stress tested with "jsc", which lacking a DOM would encounter an exception here. LayoutTests/fast/dom may be a better place? > Source/JavaScriptCore/ChangeLog:32 > + (JSC::JSObject::getOwnPropertyDescriptor): extends the case for Incomplete thought here. > Source/JavaScriptCore/runtime/JSAnchoredFunction.cpp:94 > + > + Style: Extra whitespace. > Source/JavaScriptCore/runtime/JSAnchoredFunction.h:2 > + * Copyright (C) 2011 Apple Inc. All rights reserved. Nit: 2015. > Source/JavaScriptCore/runtime/JSAnchoredFunction.h:36 > + > + > + Style: Extra whitespace. > Source/JavaScriptCore/runtime/JSAnchoredFunction.h:44 > + enum class Type { Getter = 0, Setter = 1}; Style: Weird whitespace. > Source/JavaScriptCore/runtime/JSObject.cpp:2494 > + auto key = std::make_pair(getterSetter, (bool)type); This conversion from Enum into bool is risky. If a new enum value was added this might produce unexpected results.
(In reply to comment #14) > Also, since this test references DOM objects (Node) I don't think it should > live in LayoutTests/js. I believe LayoutTests/js/script-tests are either > performance / stress tested with "jsc", which lacking a DOM would encounter > an exception here. LayoutTests/fast/dom may be a better place? Put it in Layout/js/dom.
Note, there is a comment in Source/JavaScriptCore/inspector/InjectedScriptSource.js that pertains to this. Please make sure that if this lands the Web Inspector still has a reasonable display for something like a MouseEvent. Note that when I made native descriptors include functions I special cased them in InjectedScriptSource to make native getters/setters appear in the inspector as properties on the Object instead of accessors that must be invoked in the prototype chain. See that logic in: https://bugs.webkit.org/attachment.cgi?id=246689&action=review Let me know if you have any questions!
This already sorta started with Regex's moving their properties to the prototype chain and having real descriptors =)
(In reply to comment #14) > Comment on attachment 255122 [details] > [Patch] New proposed fix > > The console.log's don't appear to output useful data, only the first string > parameter. I'm not outputting it to the console so that there will be useful data, I'm outputting it to ensure it doesn't crash JSC, which was a problem I had when writing this. I'll make this clearer in the code. > At the very least we should compare that getting the getter multiple times > is the same getter, which is the purpose of the WeakGCMap on VM: > > var getter1 = Object.getOwnPropertyDescriptor(Node.prototype, > 'nodeType').get; > var getter2 = Object.getOwnPropertyDescriptor(Node.prototype, > 'nodeType').get; > shouldBeTrue("getter1 === getter2"); > > I'd recommend adding some of the tests from my original, obsoleted patch. It > tested a number of different bindings generated in different ways, and > properties of the resulting functions (toString, length, identity) not > covered here. > fast/dom/document-property-descriptors.html / > fast/dom/event-property-descriptors.html > https://bugs.webkit.org/attachment.cgi?id=246689&action=review wilco > Also, since this test references DOM objects (Node) I don't think it should > live in LayoutTests/js. I believe LayoutTests/js/script-tests are either > performance / stress tested with "jsc", which lacking a DOM would encounter > an exception here. LayoutTests/fast/dom may be a better place? > > Source/JavaScriptCore/ChangeLog:32 > > + (JSC::JSObject::getOwnPropertyDescriptor): extends the case for > This conversion from Enum into bool is risky. If a new enum value was added > this might produce unexpected results. sure.
Created attachment 255235 [details] [Patch] New proposed fix Added more detailed test case and fixed the inspector's display to work on native getters.
Comment on attachment 255235 [details] [Patch] New proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=255235&action=review Inspector parts and tests look good to me! > Source/JavaScriptCore/inspector/InjectedScriptSource.js:63 > +function endsWith(str, suffix) > +{ > + var position = str.length - suffix.length; > + if (position < 0) > + return false; > + return str.indexOf(suffix, position) === position; > +} Given we use String.prototype.indexOf maybe we should just use String.prototype.endsWith below and drop this helper. > Source/JavaScriptCore/inspector/InjectedScriptSource.js:675 > + function descriptorHasSetterOrGetter(descriptor, setOrGet) > + { > + if (!descriptor[setOrGet]) > + return descriptor.hasOwnProperty(setOrGet); > + return descriptor.get && toString(descriptor.get).endsWith("[native code]\n}"); > + } This is unused, seems you inlined most of it. You can get rid of it.
Comment on attachment 255235 [details] [Patch] New proposed fix Attachment 255235 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5323629134872576 New failing tests: inspector-protocol/runtime/getProperties.html
Created attachment 255249 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 255235 [details] [Patch] New proposed fix Attachment 255235 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4945568946192384 New failing tests: inspector-protocol/runtime/getProperties.html
Created attachment 255251 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 255362 [details] [Patch] New proposed fix Fixed the test failure which was discussed as of https://bugs.webkit.org/show_bug.cgi?id=140575#c8
Comment on attachment 255362 [details] [Patch] New proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=255362&action=review r=me with recommended changes and fixes. > LayoutTests/js/dom/script-tests/native-bindings-descriptors.js:43 > +// tests that logging one of these to the console does not cause a crash or throw an exception. Let’s put this comment above line 38. > Source/JavaScriptCore/ChangeLog:28 > + (JSC::JSGlobalObject::init): Added a globaly initialized structure for JSAnchoredFunction typo: globally. > Source/JavaScriptCore/inspector/InjectedScriptSource.js:676 > + function descriptorHasSetterOrGetter(descriptor, setOrGet) > + { > + if (!descriptor[setOrGet]) > + return descriptor.hasOwnProperty(setOrGet); > + return descriptor.get && toString(descriptor.get).endsWith("[native code]\n}"); > + } > + Please remove because it’s unused. > Source/JavaScriptCore/runtime/JSAnchoredFunction.cpp:41 > +EncodedJSValue JSC_HOST_CALL anchoredFunctionCall(ExecState* exec) > +{ > + JSAnchoredFunction* anchoredFunction = jsCast<JSAnchoredFunction*>(exec->callee()); > + JSObject* baseObject = anchoredFunction->anchor(); Looks like the meaning of the “anchor” here is that you want to bind the function to a specific SlotBase pointer. Let’s call JSAnchoredFunction JSBoundSlotBaseFunction instead. I think that that better describes what it does. > Source/JavaScriptCore/runtime/JSAnchoredFunction.cpp:60 > + , m_anchor(vm, this, anchor) > + , m_getterSetter(vm, this, getterSetter) Let’s do these initialization in finishCreation() because they are non-trivial. > Source/JavaScriptCore/runtime/JSObject.cpp:2495 > + JSObject* anchoredObj = exec->vm().customGetterSetterFunctionMap.get(key); I see no reason why this customGetterSetterFunctionMap’s value should be a JSObject* since it is only used to store JSAnchoredFunction (or JSBoundSlotBaseFunction) pointers. Let’s change it to reflect that. > Source/JavaScriptCore/runtime/JSObject.cpp:2518 > + JSValue maybeGetterSetter = getDirect(exec->vm(), propertyName); > + CustomGetterSetter* getterSetter; > + if (maybeGetterSetter && (getterSetter = jsCast<CustomGetterSetter*>(maybeGetterSetter))) { If we get here, then we know that this property is a CustomAccessor. Hence, we should be able to just jsCast<CustomGetterSetter*> it.
Created attachment 255432 [details] [Patch] Responded to last review Testing full build before commit.
Attachment 255432 [details] did not pass style-queue: ERROR: LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 255436 [details] [Patch] Responded to last review fixed changelog bugs in last submission
Created attachment 255438 [details] [Patch] Responded to last review more patch fixes.
Comment on attachment 255438 [details] [Patch] Responded to last review Landed in http://trac.webkit.org/changeset/185889
Closing bug
(In reply to comment #31) > Comment on attachment 255438 [details] > [Patch] Responded to last review > > Landed in http://trac.webkit.org/changeset/185889 It made 2 tests crash on the debug bots.
(In reply to comment #33) > (In reply to comment #31) > > Comment on attachment 255438 [details] > > [Patch] Responded to last review > > > > Landed in http://trac.webkit.org/changeset/185889 > > It made 2 tests crash on the debug bots. I’m looking into this now. Looks like just a couple of bad asserts. Currently testing to see.
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #31) > > > Comment on attachment 255438 [details] > > > [Patch] Responded to last review > > > > > > Landed in http://trac.webkit.org/changeset/185889 > > > > It made 2 tests crash on the debug bots. > > I’m looking into this now. Looks like just a couple of bad asserts. > Currently testing to see. Fixed in r185902: <http://trac.webkit.org/r185902>.
https://bugs.webkit.org/show_bug.cgi?id=146528 As per the regression on chromeexperiments.com, I think configurable should be set to true.
(In reply to comment #36) > https://bugs.webkit.org/show_bug.cgi?id=146528 > > As per the regression on chromeexperiments.com, I think configurable should > be set to true. That would be a change in our own behavior. We also encountered this kind of issue (bindings should be made configurable) with Skydive (Microsoft Live): https://bugs.webkit.org/show_bug.cgi?id=144373 Which caused us to rollout this change: https://bugs.webkit.org/show_bug.cgi?id=142934
But I fully support making them configurable. See my comment: https://bugs.webkit.org/show_bug.cgi?id=144373#c1
Looks like the original change was rolled out in bug 146528 with: http://trac.webkit.org/changeset/186202
Comment on attachment 255362 [details] [Patch] New proposed fix Cleared Mark Lam's review+ from obsolete attachment 255362 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Attributes were made configurable in: https://bugs.webkit.org/show_bug.cgi?id=134364 I think we can give this another shot.
Created attachment 270151 [details] Patch
Created attachment 270153 [details] Patch
Comment on attachment 270151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270151&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2562 > + JSValue maybeGetterSetter = getDirect(exec->vm(), propertyName); > + if (maybeGetterSetter) { > + > + CustomGetterSetter* getterSetter = jsCast<CustomGetterSetter*>(maybeGetterSetter); > + ASSERT(getterSetter); Style: You could simplify these lines and eliminate the assert. JSValue maybeGetterSetter = getDirect(exec->vm(), propertyName); if (CustomGetterSetter* getterSetter = jsDynamicCast<CustomGetterSetter*>(maybeGetterSetter)) { ... } > Source/JavaScriptCore/runtime/JSObject.cpp:2568 > + descriptor.setEnumerable(true); Line 2556 copies the attributes to the resulting descriptor (things like DontEnum (enumerable) etc). Even before this fix, the descriptor for bindings was already showing up as enumerable correctly (e.g. Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey")). Is the explicit setEnumerable(true) here needed? What happens if you just remove this one line? I'm sure there are some WebIDL methods that might produce something non-enumerable via [NotEnumerable]. If there is such a case this would likely be wrong.
Comment on attachment 270153 [details] Patch What is the performance impact of this? Could we also add tests to make sure that getDescriptor("foo")===getDescriptor("foo") and setDescriptor("foo", f);getDescriptor("foo") === f; etc,etc
(In reply to comment #45) > Comment on attachment 270153 [details] > Patch > > What is the performance impact of this? This only modifies the performance of Object.getOwnPropertyDescriptor if you did it on a CustomAccessor (generated binding), which I expect would be super rare. > Could we also add tests to make sure that > getDescriptor("foo")===getDescriptor("foo") and setDescriptor("foo", f);getDescriptor("foo") === f; I'm pretty sure in the spec getOwnPropertyDescriptor returns a new object each time. That is certainly the case right now for JSC: var x = {foo:1}; Object.getOwnPropertyDescriptor(x, "foo") === Object.getOwnPropertyDescriptor(x, "foo") // false
Comment on attachment 270153 [details] Patch Attachment 270153 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/752581 New failing tests: inspector/runtime/getProperties.html
Created attachment 270160 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
That said, there were a bunch of tests in earlier patches that verified that the getter/setter functions themselves stay the same, and actually work when called. Where did those tests go? r- to re-include them. See the tests in: https://bugs.webkit.org/attachment.cgi?id=255438&action=review > LayoutTests/ChangeLog:12 > + * js/dom/native-bindings-descriptors-expected.txt: Added. > + * js/dom/native-bindings-descriptors.html: Added. > + * js/dom/script-tests/native-bindings-descriptors.js: Added.
(In reply to comment #46) > (In reply to comment #45) > > Comment on attachment 270153 [details] > > Patch > > > > What is the performance impact of this? > > This only modifies the performance of Object.getOwnPropertyDescriptor if you > did it on a CustomAccessor (generated binding), which I expect would be > super rare. This is my understanding as well but I'll make sure it does not regress Speedometer.
Sorry, i meant getDescriptor().get/set
Created attachment 270162 [details] Patch
Comment on attachment 270162 [details] Patch No regression on speedometer.
Created attachment 270164 [details] Patch
Comment on attachment 270164 [details] Patch Attachment 270164 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/753059 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html inspector/debugger/setBreakpoint-actions.html js/dom/native-bindings-descriptors.html fast/dom/attributes-configurable.html js/dom/xhr-prototype-define-property.html fast/xmlhttprequest/xmlhttprequest-properties-prototype.html fast/dom/webidl-operations-on-node-prototype.html
Created attachment 270170 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Here's a real example of what i expect to be true: Object.getOwnPropertyDescriptor(document.__proto__.__proto__, "body").get === Object.getOwnPropertyDescriptor(document.__proto__.__proto__, "body").get descriptor = Object.getOwnPropertyDescriptor(document.body.__proto__.__proto__.__proto__, "style") getter = descriptor.get getter.call(document.body) === document.body.style logGet = function () { console.log("calling getter"); return getter.call(this) } logSet = function (s) { console.log("calling setter"); return setter.call(this, s) } descriptor.get = logGet descriptor.set = logSet Object.defineProperty(document.body.__proto__.__proto__.__proto__, "style", descriptor) document.body.style ; //should log document.body.style = "some style" // should log, and update style Object.getOwnPropertyDescriptor(document.body.__proto__.__proto__.__proto__, "style").set == logSet Object.getOwnPropertyDescriptor(document.body.__proto__.__proto__.__proto__, "style").get == logGet
Created attachment 270279 [details] Patch
Comment on attachment 270279 [details] Patch Attachment 270279 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/757486 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html
Created attachment 270284 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270279 [details] Patch Attachment 270279 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/757582 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html
Created attachment 270285 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 270279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270279&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2559 > + if (maybeGetterSetter && maybeGetterSetter.isCustomGetterSetter()) { I haven't set the review flag yet because this patch does not seem to work yet for attributes that are on the instance (e.g. [Unforgeable] attributes such as document.location). For these attributes, getDirect(exec->vm(), propertyName) returns a null/empty JSValue() and I haven't figured out why yet. The second part of the if check (maybeGetterSetter.isCustomGetterSetter()) does not seem needed though. I have tried to reify the properties before calling getDirect() but this did not help. Please let me know if someone has an idea of what could be happening.
Comment on attachment 270279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270279&action=review >> Source/JavaScriptCore/runtime/JSObject.cpp:2559 >> + if (maybeGetterSetter && maybeGetterSetter.isCustomGetterSetter()) { > > I haven't set the review flag yet because this patch does not seem to work yet for attributes that are on the instance (e.g. [Unforgeable] attributes such as document.location). For these attributes, getDirect(exec->vm(), propertyName) returns a null/empty JSValue() and I haven't figured out why yet. The second part of the if check (maybeGetterSetter.isCustomGetterSetter()) does not seem needed though. I have tried to reify the properties before calling getDirect() but this did not help. Please let me know if someone has an idea of what could be happening. Looking at this further, this has got to mean that the property was not reified. Looking at our JS bindings, I see that their finishCreation() method reifies the static properties on the prototype: e.g. reifyStaticProperties(vm, JSTestActiveDOMObjectPrototypeTableValues, *this); But we don't do it for properties on the instance.
Comment on attachment 270279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270279&action=review >>> Source/JavaScriptCore/runtime/JSObject.cpp:2559 >>> + if (maybeGetterSetter && maybeGetterSetter.isCustomGetterSetter()) { >> >> I haven't set the review flag yet because this patch does not seem to work yet for attributes that are on the instance (e.g. [Unforgeable] attributes such as document.location). For these attributes, getDirect(exec->vm(), propertyName) returns a null/empty JSValue() and I haven't figured out why yet. The second part of the if check (maybeGetterSetter.isCustomGetterSetter()) does not seem needed though. I have tried to reify the properties before calling getDirect() but this did not help. Please let me know if someone has an idea of what could be happening. > > Looking at this further, this has got to mean that the property was not reified. Looking at our JS bindings, I see that their finishCreation() method reifies the static properties on the prototype: > e.g. reifyStaticProperties(vm, JSTestActiveDOMObjectPrototypeTableValues, *this); > > But we don't do it for properties on the instance. Looks like the eager reify of the prototype properties was done by Sam in https://bugs.webkit.org/show_bug.cgi?id=133822.
Eager reification on instance will be too expensive (that's why we only do the prototype chain reification). Individual instance creation is just very very hot :-/
(In reply to comment #66) > Eager reification on instance will be too expensive (that's why we only do > the prototype chain reification). Individual instance creation is just very > very hot :-/ I see. Well, we could reify in getOwnPropertyDescriptor() if needed as we assume this is not particularly hot code. Doing so seems to make Object.getOwnPropertyDescriptor(document, "location") work, which is good. However, it still does not work for window.location. I investigated a little and the reason it does not work is because we are getting a JSDOMWindowShell object, instead of a JSDOMWindow one. JSDOMWindowShell does not have a staticPropHashTable (no static properties to reify).
Created attachment 270429 [details] Patch
Created attachment 270438 [details] Patch
Comment on attachment 270438 [details] Patch This looks sound to me, but could you add a few tests to cover the getter/setter of properties if you call them on different objects -- just to verify that they aren't actually binding to the origin element. e.g. gOPD(someDiv, innerText).set.call(someOtherDiv, "blargh") should only modify someOtherDiv We also need to ensure the magic security objects don't leak across boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) should not expose any objects from otherWindow's object graph.
Created attachment 270445 [details] Patch
(In reply to comment #70) > Comment on attachment 270438 [details] > Patch > > This looks sound to me, but could you add a few tests to cover the > getter/setter of properties if you call them on different objects -- just to > verify that they aren't actually binding to the origin element. e.g. > > gOPD(someDiv, innerText).set.call(someOtherDiv, "blargh") > > should only modify someOtherDiv Ok, I added a 4th test to cover this. > We also need to ensure the magic security objects don't leak across > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > should not expose any objects from otherWindow's object graph. Would you mind clarifying this one? I am not very familiar with this. Several things I don't understand is that open is not an attribute but an operation. Therefore, it is not a CustomAccessor and does not have a GetterSetter. It should not be impacted by my patch.
Comment on attachment 270445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270445&action=review r=me > LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt:2346 > +FAIL TextTrack interface: attribute language assert_equals: setter must be undefined for readonly attributes expected (undefined) undefined but got (function) function "function language() { > + [native code] > +}" Are we doing the right thing for readonly properties?
Comment on attachment 270445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270445&action=review >> LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt:2346 >> +}" > > Are we doing the right thing for readonly properties? TextTrack.language is not readonly in WebKit. This is likely a separate issue.
> > We also need to ensure the magic security objects don't leak across > > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > > should not expose any objects from otherWindow's object graph. > > Would you mind clarifying this one? I am not very familiar with this. > Several things I don't understand is that open is not an attribute but an > operation. Therefore, it is not a CustomAccessor and does not have a > GetterSetter. It should not be impacted by my patch. Derp. I'm sure there's a property that has some magical behaviour -- .location or some such? Another thing i just thought about documetGetterForWindowA.call(windowB) Should throw
(In reply to comment #75) > > > We also need to ensure the magic security objects don't leak across > > > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > > > should not expose any objects from otherWindow's object graph. > > > > Would you mind clarifying this one? I am not very familiar with this. > > Several things I don't understand is that open is not an attribute but an > > operation. Therefore, it is not a CustomAccessor and does not have a > > GetterSetter. It should not be impacted by my patch. > > Derp. > > I'm sure there's a property that has some magical behaviour -- .location or > some such? > > Another thing i just thought about > > documetGetterForWindowA.call(windowB) > > Should throw I have just tried the following in Firefox: win2 = window.open("") Object.getOwnPropertyDescriptor(window, "document").get.call(win2) It did not throw and returned an HTMLDocument. However, in WebKit and Chrome, Object.getOwnPropertyDescriptor(window, "document").get is undefined because it is a value rather than a GetterSetter. I'll add a test to cover this.
(In reply to comment #76) > (In reply to comment #75) > > > > We also need to ensure the magic security objects don't leak across > > > > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > > > > should not expose any objects from otherWindow's object graph. > > > > > > Would you mind clarifying this one? I am not very familiar with this. > > > Several things I don't understand is that open is not an attribute but an > > > operation. Therefore, it is not a CustomAccessor and does not have a > > > GetterSetter. It should not be impacted by my patch. > > > > Derp. > > > > I'm sure there's a property that has some magical behaviour -- .location or > > some such? > > > > Another thing i just thought about > > > > documetGetterForWindowA.call(windowB) > > > > Should throw > > I have just tried the following in Firefox: > win2 = window.open("") > Object.getOwnPropertyDescriptor(window, "document").get.call(win2) > > It did not throw and returned an HTMLDocument. > > However, in WebKit and Chrome, Object.getOwnPropertyDescriptor(window, > "document").get is undefined because it is a value rather than a > GetterSetter. I'll add a test to cover this. sorry, you need the windows to be different origins.
(In reply to comment #77) > (In reply to comment #76) > > (In reply to comment #75) > > > > > We also need to ensure the magic security objects don't leak across > > > > > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > > > > > should not expose any objects from otherWindow's object graph. > > > > > > > > Would you mind clarifying this one? I am not very familiar with this. > > > > Several things I don't understand is that open is not an attribute but an > > > > operation. Therefore, it is not a CustomAccessor and does not have a > > > > GetterSetter. It should not be impacted by my patch. > > > > > > Derp. > > > > > > I'm sure there's a property that has some magical behaviour -- .location or > > > some such? > > > > > > Another thing i just thought about > > > > > > documetGetterForWindowA.call(windowB) > > > > > > Should throw > > > > I have just tried the following in Firefox: > > win2 = window.open("") > > Object.getOwnPropertyDescriptor(window, "document").get.call(win2) > > > > It did not throw and returned an HTMLDocument. > > > > However, in WebKit and Chrome, Object.getOwnPropertyDescriptor(window, > > "document").get is undefined because it is a value rather than a > > GetterSetter. I'll add a test to cover this. > > > sorry, you need the windows to be different origins. Ok, I will add a cross-origin test for window.location and window.document and using the property getter from another window. These are not really impacted by this patch but will throw as expected.
Created attachment 270463 [details] Patch
(In reply to comment #78) > (In reply to comment #77) > > (In reply to comment #76) > > > (In reply to comment #75) > > > > > > We also need to ensure the magic security objects don't leak across > > > > > > boundaries -- e.g. gOPD(otherWindow, "open").get/set (and the object itself) > > > > > > should not expose any objects from otherWindow's object graph. > > > > > > > > > > Would you mind clarifying this one? I am not very familiar with this. > > > > > Several things I don't understand is that open is not an attribute but an > > > > > operation. Therefore, it is not a CustomAccessor and does not have a > > > > > GetterSetter. It should not be impacted by my patch. > > > > > > > > Derp. > > > > > > > > I'm sure there's a property that has some magical behaviour -- .location or > > > > some such? > > > > > > > > Another thing i just thought about > > > > > > > > documetGetterForWindowA.call(windowB) > > > > > > > > Should throw > > > > > > I have just tried the following in Firefox: > > > win2 = window.open("") > > > Object.getOwnPropertyDescriptor(window, "document").get.call(win2) > > > > > > It did not throw and returned an HTMLDocument. > > > > > > However, in WebKit and Chrome, Object.getOwnPropertyDescriptor(window, > > > "document").get is undefined because it is a value rather than a > > > GetterSetter. I'll add a test to cover this. > > > > > > sorry, you need the windows to be different origins. > > Ok, I will add a cross-origin test for window.location and window.document > and using the property getter from another window. These are not really > impacted by this patch but will throw as expected. Added in latest patch iteration: http/tests/security/cross-origin-window-property-access.html
Comment on attachment 270463 [details] Patch Clearing flags on attachment: 270463 Committed r196001: <http://trac.webkit.org/changeset/196001>
All reviewed patches have been landed. Closing bug.
*** Bug 154458 has been marked as a duplicate of this bug. ***