Changeset 209442 in webkit

Timestamp:

Dec 6, 2016 7:12:05 PM (7 years ago)

Author:

mark.lam@apple.com

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

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/js/script-tests/prototype-assignment.js (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp (modified) (6 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/TestExpectations

@@ -641 +641 @@
 [ 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.

trunk/LayoutTests/js/script-tests/prototype-assignment.js

@@ -1 @@
-//@ 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

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp

@@ -886 +886 @@
     case CustomValueSetter:
     case CustomAccessorSetter: {
+        GPRReg valueRegsPayloadGPR = valueRegs.payloadGPR();
+        
         if (isValidOffset(m_offset)) {
             Structure* currStructure;
@@ -897 +899 @@
         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()),
@@ -916 +926 @@
         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;
@@ -987 +1000 @@
 
         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:
             //
@@ -1065 +1081 @@
 
             jit.storeCell(
-                baseForGetGPR,
+                baseGPR,
                 calleeFrame.withOffset(virtualRegisterForArgument(0).offset() * sizeof(Register)));
 
@@ -1119 +1135 @@
                 });
         } 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