Title: [209442] trunk
- Revision
- 209442
- Author
- mark....@apple.com
- Date
- 2016-12-06 19:12:05 -0800 (Tue, 06 Dec 2016)
Log Message
GetByID IC is wrongly unwrapping the global proxy this value for getter/setters.
https://bugs.webkit.org/show_bug.cgi?id=165401
Reviewed by Saam Barati.
Source/_javascript_Core:
When the this value for a property access is the JS global and that property
access is via a GetterSetter, the underlying getter / setter functions would
expect the this value they receive to be the JSProxy instance instead of the
JSGlobalObject. This is consistent with how the LLINT and runtime code behaves.
The IC code should behave the same way.
Also added some ASSERTs to document invariants in the code, and help detect
bugs sooner if the code gets changed in a way that breaks those invariants in
the future.
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::generateImpl):
LayoutTests:
Set the test loose now that this bug is fixed.
* TestExpectations:
* js/script-tests/prototype-assignment.js:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (209441 => 209442)
--- trunk/LayoutTests/ChangeLog 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/LayoutTests/ChangeLog 2016-12-07 03:12:05 UTC (rev 209442)
@@ -1,3 +1,15 @@
+2016-12-06 Mark Lam <mark....@apple.com>
+
+ GetByID IC is wrongly unwrapping the global proxy this value for getter/setters.
+ https://bugs.webkit.org/show_bug.cgi?id=165401
+
+ Reviewed by Saam Barati.
+
+ Set the test loose now that this bug is fixed.
+
+ * TestExpectations:
+ * js/script-tests/prototype-assignment.js:
+
2016-12-06 Dean Jackson <d...@apple.com>
Apply styling to media documents with modern controls
Modified: trunk/LayoutTests/TestExpectations (209441 => 209442)
--- trunk/LayoutTests/TestExpectations 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/LayoutTests/TestExpectations 2016-12-07 03:12:05 UTC (rev 209442)
@@ -640,8 +640,6 @@
[ Debug ] js/regress-141098.html [ Slow ]
-webkit.org/b/165401 js/prototype-assignment.html [ Skip ]
-
# IDBVersionChangeEvent tests need to be rewritten to use event constructors instead of createEvent,
# after we implement the IDBVersionChangeEvent constructor.
webkit.org/b/145390 storage/indexeddb/events.html [ Failure ]
Modified: trunk/LayoutTests/js/script-tests/prototype-assignment.js (209441 => 209442)
--- trunk/LayoutTests/js/script-tests/prototype-assignment.js 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/LayoutTests/js/script-tests/prototype-assignment.js 2016-12-07 03:12:05 UTC (rev 209442)
@@ -1,5 +1,4 @@
-//@ runFTLNoCJIT("--useJIT=false")
-// FIXME: Remove the "--useJIT=false" option when https://bugs.webkit.org/show_bug.cgi?id=165401 is fixed.
+//@ runFTLNoCJIT
// This test suite compares the behavior of setting the prototype on various values
// (using Object.setPrototypeOf(), obj.__proto__ assignment, and Reflect.setPrototypeOf())
Modified: trunk/Source/_javascript_Core/ChangeLog (209441 => 209442)
--- trunk/Source/_javascript_Core/ChangeLog 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-12-07 03:12:05 UTC (rev 209442)
@@ -1,3 +1,23 @@
+2016-12-06 Mark Lam <mark....@apple.com>
+
+ GetByID IC is wrongly unwrapping the global proxy this value for getter/setters.
+ https://bugs.webkit.org/show_bug.cgi?id=165401
+
+ Reviewed by Saam Barati.
+
+ When the this value for a property access is the JS global and that property
+ access is via a GetterSetter, the underlying getter / setter functions would
+ expect the this value they receive to be the JSProxy instance instead of the
+ JSGlobalObject. This is consistent with how the LLINT and runtime code behaves.
+ The IC code should behave the same way.
+
+ Also added some ASSERTs to document invariants in the code, and help detect
+ bugs sooner if the code gets changed in a way that breaks those invariants in
+ the future.
+
+ * bytecode/PolymorphicAccess.cpp:
+ (JSC::AccessCase::generateImpl):
+
2016-12-06 Joseph Pecoraro <pecor...@apple.com>
DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots
Modified: trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp (209441 => 209442)
--- trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/Source/_javascript_Core/bytecode/PolymorphicAccess.cpp 2016-12-07 03:12:05 UTC (rev 209442)
@@ -885,6 +885,8 @@
case CustomAccessorGetter:
case CustomValueSetter:
case CustomAccessorSetter: {
+ GPRReg valueRegsPayloadGPR = valueRegs.payloadGPR();
+
if (isValidOffset(m_offset)) {
Structure* currStructure;
if (m_conditionSet.isEmpty())
@@ -896,7 +898,15 @@
GPRReg baseForGetGPR;
if (viaProxy()) {
- baseForGetGPR = valueRegs.payloadGPR();
+ ASSERT(m_type != CustomValueSetter || m_type != CustomAccessorSetter); // Because setters need to not trash valueRegsPayloadGPR.
+ if (m_type == Getter || m_type == Setter)
+ baseForGetGPR = scratchGPR;
+ else
+ baseForGetGPR = valueRegsPayloadGPR;
+
+ ASSERT((m_type != Getter && m_type != Setter) || baseForGetGPR != baseGPR);
+ ASSERT(m_type != Setter || baseForGetGPR != valueRegsPayloadGPR);
+
jit.loadPtr(
CCallHelpers::Address(baseGPR, JSProxy::targetOffset()),
baseForGetGPR);
@@ -915,10 +925,13 @@
GPRReg loadedValueGPR = InvalidGPRReg;
if (m_type != CustomValueGetter && m_type != CustomAccessorGetter && m_type != CustomValueSetter && m_type != CustomAccessorSetter) {
if (m_type == Load || m_type == GetGetter)
- loadedValueGPR = valueRegs.payloadGPR();
+ loadedValueGPR = valueRegsPayloadGPR;
else
loadedValueGPR = scratchGPR;
+ ASSERT((m_type != Getter && m_type != Setter) || loadedValueGPR != baseGPR);
+ ASSERT(m_type != Setter || loadedValueGPR != valueRegsPayloadGPR);
+
GPRReg storageGPR;
if (isInlineOffset(m_offset))
storageGPR = baseForAccessGPR;
@@ -986,6 +999,9 @@
CCallHelpers::tagFor(static_cast<VirtualRegister>(CallFrameSlot::argumentCount)));
if (m_type == Getter || m_type == Setter) {
+ ASSERT(baseGPR != loadedValueGPR);
+ ASSERT(m_type != Setter || (baseGPR != valueRegsPayloadGPR && loadedValueGPR != valueRegsPayloadGPR));
+
// Create a JS call using a JS call inline cache. Assume that:
//
// - SP is aligned and represents the extent of the calling compiler's stack usage.
@@ -1064,7 +1080,7 @@
loadedValueGPR, calleeFrame.withOffset(CallFrameSlot::callee * sizeof(Register)));
jit.storeCell(
- baseForGetGPR,
+ baseGPR,
calleeFrame.withOffset(virtualRegisterForArgument(0).offset() * sizeof(Register)));
if (m_type == Setter) {
@@ -1118,6 +1134,8 @@
CodeLocationLabel(vm.getCTIStub(linkCallThunkGenerator).code()));
});
} else {
+ ASSERT(m_type == CustomValueGetter || m_type == CustomAccessorGetter || m_type == CustomValueSetter || m_type == CustomAccessorSetter);
+
// Need to make room for the C call so any of our stack spillage isn't overwritten. It's
// hard to track if someone did spillage or not, so we just assume that we always need
// to make some space here.
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
