Native Bindings Descriptors are Incomplete

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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}

[Radar WebKit Bug Importer]

<rdar://problem/19506502>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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)

[Joseph Pecoraro]

Created attachment 246565 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

Created attachment 246689 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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.

[Matthew Mirman]

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.

[Matthew Mirman]

Created attachment 255122 [details]
[Patch] New proposed fix

fixed build problem and added test expectations.

[Joseph Pecoraro]

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.

[Mark Lam]

(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.

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

This already sorta started with Regex's moving their properties to the prototype chain and having real descriptors =)

[Matthew Mirman]

(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.

[Matthew Mirman]

Created attachment 255235 [details]
[Patch] New proposed fix

Added more detailed test case and fixed the inspector's display to work on native getters.

[Joseph Pecoraro]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matthew Mirman]

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

[Mark Lam]

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.

[Matthew Mirman]

Created attachment 255432 [details]
[Patch] Responded to last review

Testing full build before commit.

[WebKit Commit Bot]

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.

[Matthew Mirman]

Created attachment 255436 [details]
[Patch] Responded to last review

fixed changelog bugs in last submission

[Matthew Mirman]

Created attachment 255438 [details]
[Patch] Responded to last review

more patch fixes.

[Matthew Mirman]

Comment on attachment 255438 [details]
[Patch] Responded to last review

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

[Matthew Mirman]

Closing bug

[Csaba Osztrogonác]

(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.

[Mark Lam]

(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.

[Mark Lam]

(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>.

[Matthew Mirman]

https://bugs.webkit.org/show_bug.cgi?id=146528  

As per the regression on chromeexperiments.com, I think configurable should be set to true.

[Joseph Pecoraro]

(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

[Joseph Pecoraro]

But I fully support making them configurable. See my comment:
https://bugs.webkit.org/show_bug.cgi?id=144373#c1

[Joseph Pecoraro]

Looks like the original change was rolled out in bug 146528 with:
http://trac.webkit.org/changeset/186202

[Csaba Osztrogonác]

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.

[Chris Dumez]

Attributes were made configurable in:
https://bugs.webkit.org/show_bug.cgi?id=134364

I think we can give this another shot.

[Chris Dumez]

Created attachment 270151 [details]
Patch

[Chris Dumez]

Created attachment 270153 [details]
Patch

[Joseph Pecoraro]

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.

[Oliver Hunt]

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

[Joseph Pecoraro]

(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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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.

[Chris Dumez]

(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.

[Oliver Hunt]

Sorry, i meant getDescriptor().get/set

[Chris Dumez]

Created attachment 270162 [details]
Patch

[Chris Dumez]

Comment on attachment 270162 [details]
Patch

No regression on speedometer.

[Chris Dumez]

Created attachment 270164 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Oliver Hunt]

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

[Chris Dumez]

Created attachment 270279 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Oliver Hunt]

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 :-/

[Chris Dumez]

(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).

[Chris Dumez]

Created attachment 270429 [details]
Patch

[Chris Dumez]

Created attachment 270438 [details]
Patch

[Oliver Hunt]

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.

[Chris Dumez]

Created attachment 270445 [details]
Patch

[Chris Dumez]

(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.

[Oliver Hunt]

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?

[Chris Dumez]

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.

[Oliver Hunt]

> > 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

[Chris Dumez]

(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.

[Oliver Hunt]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 270463 [details]
Patch

[Chris Dumez]

(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

[WebKit Commit Bot]

Comment on attachment 270463 [details]
Patch

Clearing flags on attachment: 270463

Committed r196001: <http://trac.webkit.org/changeset/196001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

*** Bug 154458 has been marked as a duplicate of this bug. ***