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
Created attachment 287541 [details] Patch
This is covered by: http://w3c-test.org/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html
Comment on attachment 287541 [details] Patch r=me
Comment on attachment 287541 [details] Patch Clearing flags on attachment: 287541 Committed r205297: <http://trac.webkit.org/changeset/205297>
All reviewed patches have been landed. Closing bug.
Reverted r205297 for reason: Caused some JSC test failures Committed r205301: <http://trac.webkit.org/changeset/205301>
Created attachment 287728 [details] Patch
Created attachment 287730 [details] Patch
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.
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
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
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
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
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
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
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
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
Created attachment 287762 [details] Patch
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.
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.
Created attachment 287773 [details] Patch
Comment on attachment 287773 [details] Patch Clearing flags on attachment: 287773 Committed r205354: <http://trac.webkit.org/changeset/205354>
*** Bug 161508 has been marked as a duplicate of this bug. ***
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.
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.
(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 :/
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.
Reopen to deal with JSC test failures.
Created attachment 287813 [details] Follow-up fix Still running the tests locally.
Created attachment 287815 [details] Follow-up fix
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.
Rolled out in <http://trac.webkit.org/changeset/205372>.
Created attachment 288139 [details] Patch
Created attachment 288146 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 288163 [details] Patch
Created attachment 288176 [details] Patch
I have verified that this passes the JSC tests now that the fix for Bug 161558 has landed.
Any feedback from JSC people?
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?
(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.
(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.
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?
Created attachment 288321 [details] Patch
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
Created attachment 288325 [details] Patch
Created attachment 288328 [details] Patch
Comment on attachment 288328 [details] Patch Clearing flags on attachment: 288328 Committed r205670: <http://trac.webkit.org/changeset/205670>