GetByID IC is wrongly unwrapping the global proxy this value for getter/setters.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Dec 2016 03:12:05 +0000 (03:12 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Dec 2016 03:12:05 +0000 (03:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165401

Reviewed by Saam Barati.

Source/JavaScriptCore:

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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@209442 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/js/script-tests/prototype-assignment.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp

index 194b173..2cd8786 100644 (file)
@@ -1,3 +1,15 @@
+2016-12-06  Mark Lam  <mark.lam@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  <dino@apple.com>
 
         Apply styling to media documents with modern controls
index 13e2f4d..d41849b 100644 (file)
@@ -640,8 +640,6 @@ webkit.org/b/142937 ietestcenter/Javascript/15.2.3.14-1-3.html [ Failure ]
 
 [ 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 ]
index 54a959a..aa49aa5 100644 (file)
@@ -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())
index ef0e594..8bba55b 100644 (file)
@@ -1,3 +1,23 @@
+2016-12-06  Mark Lam  <mark.lam@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  <pecoraro@apple.com>
 
         DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots
index b9757ea..5b79ae4 100644 (file)
@@ -885,6 +885,8 @@ void AccessCase::generateImpl(AccessGenerationState& state)
     case CustomAccessorGetter:
     case CustomValueSetter:
     case CustomAccessorSetter: {
+        GPRReg valueRegsPayloadGPR = valueRegs.payloadGPR();
+        
         if (isValidOffset(m_offset)) {
             Structure* currStructure;
             if (m_conditionSet.isEmpty())
@@ -896,7 +898,15 @@ void AccessCase::generateImpl(AccessGenerationState& state)
 
         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 @@ void AccessCase::generateImpl(AccessGenerationState& state)
         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 @@ void AccessCase::generateImpl(AccessGenerationState& state)
             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 @@ void AccessCase::generateImpl(AccessGenerationState& state)
                 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 @@ void AccessCase::generateImpl(AccessGenerationState& state)
                         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.