Bug 161455 – Align proto getter / setter behavior with other browsers

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Align cross-origin proto getter / setter behavior with the specification:

setter:
- https://html.spec.whatwg.org/#windowproxy-setprototypeof
- https://html.spec.whatwg.org/#location-setprototypeof
- https://tc39.github.io/ecma262/#sec-object.setprototypeof (step 5)

getter:
- https://html.spec.whatwg.org/#windowproxy-getprototypeof
- https://html.spec.whatwg.org/#location-getprototypeof

[Chris Dumez]

Created attachment 287541 [details]
Patch

[Chris Dumez]

This is covered by:
http://w3c-test.org/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html

[Mark Lam]

Comment on attachment 287541 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 287541 [details]
Patch

Clearing flags on attachment: 287541

Committed r205297: <http://trac.webkit.org/changeset/205297>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r205297 for reason:

Caused some JSC test failures

Committed r205301: <http://trac.webkit.org/changeset/205301>

[Chris Dumez]

Created attachment 287728 [details]
Patch

[Chris Dumez]

Created attachment 287730 [details]
Patch

[Chris Dumez]

Comment on attachment 287730 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1382
> +        // if (UNLIKELY(asObject(nextPrototype)->methodTable(vm)->getPrototype != JSObject::getPrototype))

I am proposing to comment this out for now. There are a lot of places in the code base where we iterate over the prototype chain and do not handle cycles. Allowing for cycles would be a significant amount of work.

[Build Bot]

Comment on attachment 287730 [details]
Patch

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

New failing tests:
fast/dom/Window/window-custom-prototype.html
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html
fast/dom/Window/window-custom-prototype-crash.html

[Build Bot]

Created attachment 287737 [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 287730 [details]
Patch

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

New failing tests:
fast/dom/Window/window-custom-prototype.html
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html
fast/dom/Window/window-custom-prototype-crash.html

[Build Bot]

Created attachment 287739 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287730 [details]
Patch

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

New failing tests:
fast/dom/Window/window-custom-prototype.html
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html
fast/dom/Window/window-custom-prototype-crash.html

[Build Bot]

Created attachment 287740 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287730 [details]
Patch

Attachment 287730 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1991980

New failing tests:
fast/dom/Window/window-custom-prototype.html
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html
fast/dom/Window/window-custom-prototype-crash.html

[Build Bot]

Created attachment 287745 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

Created attachment 287762 [details]
Patch

[Chris Dumez]

Comment on attachment 287762 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1380
> +        // FIXME: The specification says we should do this but this allows for cycles and our

I filed https://bugs.webkit.org/show_bug.cgi?id=161534 to track this.

[Mark Lam]

Comment on attachment 287762 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:52
> +        html/browsers/history/the-location-interface/allow_prototype_cycle_through_location.sub.html

Please add the https://bugs.webkit.org/show_bug.cgi?id=161534 url after this for reference.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1380
>> +        // FIXME: The specification says we should do this but this allows for cycles and our
> 
> I filed https://bugs.webkit.org/show_bug.cgi?id=161534 to track this.

Please add the bug url in the comment here.

[Chris Dumez]

Created attachment 287773 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287773 [details]
Patch

Clearing flags on attachment: 287773

Committed r205354: <http://trac.webkit.org/changeset/205354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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

[Saam Barati]

Comment on attachment 287773 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1384
> +        // if (UNLIKELY(asObject(nextPrototype)->methodTable(vm)->getPrototype != JSObject::getPrototype))
> +        //    break; // We're done. Set the prototype.

I don't think this is how we land changes in JSC. We don't turn things off to allow for other features to be added in WebCore. I think the onus is on your patch to ensure that anything new you're adding is safe.

[Geoffrey Garen]

Comment on attachment 287773 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.cpp:1384
>> +        //    break; // We're done. Set the prototype.
> 
> I don't think this is how we land changes in JSC. We don't turn things off to allow for other features to be added in WebCore. I think the onus is on your patch to ensure that anything new you're adding is safe.

Behavior decision notwithstanding, we definitely don't want to land a paragraph of commented-out code.

[Chris Dumez]

(In reply to comment #25)
> Comment on attachment 287773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287773&action=review
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:1384
> > +        // if (UNLIKELY(asObject(nextPrototype)->methodTable(vm)->getPrototype != JSObject::getPrototype))
> > +        //    break; // We're done. Set the prototype.
> 
> I don't think this is how we land changes in JSC. We don't turn things off
> to allow for other features to be added in WebCore. I think the onus is on
> your patch to ensure that anything new you're adding is safe.

Updating the code base to properly deal with cycles is not a trivial task and there are performance implications :/

[Saam Barati]

Comment on attachment 287773 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSObject.cpp:1384
>>>> +        //    break; // We're done. Set the prototype.
>>> 
>>> I don't think this is how we land changes in JSC. We don't turn things off to allow for other features to be added in WebCore. I think the onus is on your patch to ensure that anything new you're adding is safe.
>> 
>> Behavior decision notwithstanding, we definitely don't want to land a paragraph of commented-out code.
> 
> Updating the code base to properly deal with cycles is not a trivial task and there are performance implications :/

I understand. Maybe this is the correct decision. However, I think we need to have a deeper discussion about this to fully realize all the implications.

[Chris Dumez]

Reopen to deal with JSC test failures.

[Chris Dumez]

Created attachment 287813 [details]
Follow-up fix

Still running the tests locally.

[Chris Dumez]

Reopen to deal with JSC test failures.

[Chris Dumez]

Created attachment 287815 [details]
Follow-up fix

[WebKit Commit Bot]

Attachment 287815 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Rolled out in <http://trac.webkit.org/changeset/205372>.

[Chris Dumez]

Created attachment 288139 [details]
Patch

[Chris Dumez]

Created attachment 288146 [details]
Patch

[Build Bot]

Comment on attachment 288146 [details]
Patch

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

New failing tests:
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html

[Build Bot]

Created attachment 288157 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288146 [details]
Patch

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

New failing tests:
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html

[Build Bot]

Created attachment 288159 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288146 [details]
Patch

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

New failing tests:
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html

[Build Bot]

Created attachment 288160 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288146 [details]
Patch

Attachment 288146 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2026482

New failing tests:
js/object-literal-shorthand-construction.html
js/sloppy-getter-setter-global-object.html

[Build Bot]

Created attachment 288161 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

Created attachment 288163 [details]
Patch

[Chris Dumez]

Created attachment 288176 [details]
Patch

[Chris Dumez]

I have verified that this passes the JSC tests now that the fix for Bug 161558 has landed.

[Chris Dumez]

Any feedback from JSC people?

[Saam Barati]

Comment on attachment 288176 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1313
> +        // FIXME: The specification says we should do this but this allows for cycles and our
> +        // code base currently does not deal properly with such cycles.
> +        // https://bugs.webkit.org/show_bug.cgi?id=161534

Any word from the folks who work on the html spec on this?

[Chris Dumez]

(In reply to comment #49)
> Comment on attachment 288176 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288176&action=review
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:1313
> > +        // FIXME: The specification says we should do this but this allows for cycles and our
> > +        // code base currently does not deal properly with such cycles.
> > +        // https://bugs.webkit.org/show_bug.cgi?id=161534
> 
> Any word from the folks who work on the html spec on this?

Domenic, an HTML spec editor, already commented on the bug you filed against EcmaScript.

[Chris Dumez]

(In reply to comment #50)
> (In reply to comment #49)
> > Comment on attachment 288176 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288176&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:1313
> > > +        // FIXME: The specification says we should do this but this allows for cycles and our
> > > +        // code base currently does not deal properly with such cycles.
> > > +        // https://bugs.webkit.org/show_bug.cgi?id=161534
> > 
> > Any word from the folks who work on the html spec on this?
> 
> Domenic, an HTML spec editor, already commented on the bug you filed against
> EcmaScript.

That said, I filed https://github.com/whatwg/html/issues/1760 to track the issue on HTML side as well.

[Chris Dumez]

Comment on attachment 288176 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSObject.cpp:1313
>>>> +        // https://bugs.webkit.org/show_bug.cgi?id=161534
>>> 
>>> Any word from the folks who work on the html spec on this?
>> 
>> Domenic, an HTML spec editor, already commented on the bug you filed against EcmaScript.
> 
> That said, I filed https://github.com/whatwg/html/issues/1760 to track the issue on HTML side as well.

If you'd prefer, we could do something like:
if (UNLIKELY(nextPrototype->type() == ProxyObjectType))
    break; // We're done. Set the prototype.

to maintain the previous behavior. Only ProxyObject in JSC has a custom [[GetPrototypeOf]] it seems.

What do you think?

[Chris Dumez]

Created attachment 288321 [details]
Patch

[Saam Barati]

Comment on attachment 288321 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:365
> +        throwTypeError(exec, scope, ASCIILiteral("Cannot set prototype of this object"));

Nit: instead of "this object" maybe it's worth having a more detailed error message?

> Source/WebCore/bindings/js/JSLocationCustom.cpp:145
> +        throwTypeError(exec, scope, ASCIILiteral("Cannot set prototype of this object"));

ditto

[Chris Dumez]

Created attachment 288325 [details]
Patch

[Chris Dumez]

Created attachment 288328 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288328 [details]
Patch

Clearing flags on attachment: 288328

Committed r205670: <http://trac.webkit.org/changeset/205670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.