<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[209442] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/209442">209442</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2016-12-06 19:12:05 -0800 (Tue, 06 Dec 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestsTestExpectations">trunk/LayoutTests/TestExpectations</a></li>
<li><a href="#trunkLayoutTestsjsscripttestsprototypeassignmentjs">trunk/LayoutTests/js/script-tests/prototype-assignment.js</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp">trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (209441 => 209442)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/LayoutTests/ChangeLog 2016-12-07 03:12:05 UTC (rev 209442)
</span><span class="lines">@@ -1,3 +1,15 @@
</span><ins>+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:
+
</ins><span class="cx"> 2016-12-06 Dean Jackson <dino@apple.com>
</span><span class="cx">
</span><span class="cx"> Apply styling to media documents with modern controls
</span></span></pre></div>
<a id="trunkLayoutTestsTestExpectations"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/TestExpectations (209441 => 209442)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/TestExpectations 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/LayoutTests/TestExpectations 2016-12-07 03:12:05 UTC (rev 209442)
</span><span class="lines">@@ -640,8 +640,6 @@
</span><span class="cx">
</span><span class="cx"> [ Debug ] js/regress-141098.html [ Slow ]
</span><span class="cx">
</span><del>-webkit.org/b/165401 js/prototype-assignment.html [ Skip ]
-
</del><span class="cx"> # IDBVersionChangeEvent tests need to be rewritten to use event constructors instead of createEvent,
</span><span class="cx"> # after we implement the IDBVersionChangeEvent constructor.
</span><span class="cx"> webkit.org/b/145390 storage/indexeddb/events.html [ Failure ]
</span></span></pre></div>
<a id="trunkLayoutTestsjsscripttestsprototypeassignmentjs"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/js/script-tests/prototype-assignment.js (209441 => 209442)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,5 +1,4 @@
</span><del>-//@ runFTLNoCJIT("--useJIT=false")
-// FIXME: Remove the "--useJIT=false" option when https://bugs.webkit.org/show_bug.cgi?id=165401 is fixed.
</del><ins>+//@ runFTLNoCJIT
</ins><span class="cx">
</span><span class="cx"> // This test suite compares the behavior of setting the prototype on various values
</span><span class="cx"> // (using Object.setPrototypeOf(), obj.__proto__ assignment, and Reflect.setPrototypeOf())
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (209441 => 209442)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-12-07 03:12:05 UTC (rev 209442)
</span><span class="lines">@@ -1,3 +1,23 @@
</span><ins>+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):
+
</ins><span class="cx"> 2016-12-06 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><span class="cx"> DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp (209441 => 209442)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2016-12-07 03:03:16 UTC (rev 209441)
+++ trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2016-12-07 03:12:05 UTC (rev 209442)
</span><span class="lines">@@ -885,6 +885,8 @@
</span><span class="cx"> case CustomAccessorGetter:
</span><span class="cx"> case CustomValueSetter:
</span><span class="cx"> case CustomAccessorSetter: {
</span><ins>+ GPRReg valueRegsPayloadGPR = valueRegs.payloadGPR();
+
</ins><span class="cx"> if (isValidOffset(m_offset)) {
</span><span class="cx"> Structure* currStructure;
</span><span class="cx"> if (m_conditionSet.isEmpty())
</span><span class="lines">@@ -896,7 +898,15 @@
</span><span class="cx">
</span><span class="cx"> GPRReg baseForGetGPR;
</span><span class="cx"> if (viaProxy()) {
</span><del>- baseForGetGPR = valueRegs.payloadGPR();
</del><ins>+ 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);
+
</ins><span class="cx"> jit.loadPtr(
</span><span class="cx"> CCallHelpers::Address(baseGPR, JSProxy::targetOffset()),
</span><span class="cx"> baseForGetGPR);
</span><span class="lines">@@ -915,10 +925,13 @@
</span><span class="cx"> GPRReg loadedValueGPR = InvalidGPRReg;
</span><span class="cx"> if (m_type != CustomValueGetter && m_type != CustomAccessorGetter && m_type != CustomValueSetter && m_type != CustomAccessorSetter) {
</span><span class="cx"> if (m_type == Load || m_type == GetGetter)
</span><del>- loadedValueGPR = valueRegs.payloadGPR();
</del><ins>+ loadedValueGPR = valueRegsPayloadGPR;
</ins><span class="cx"> else
</span><span class="cx"> loadedValueGPR = scratchGPR;
</span><span class="cx">
</span><ins>+ ASSERT((m_type != Getter && m_type != Setter) || loadedValueGPR != baseGPR);
+ ASSERT(m_type != Setter || loadedValueGPR != valueRegsPayloadGPR);
+
</ins><span class="cx"> GPRReg storageGPR;
</span><span class="cx"> if (isInlineOffset(m_offset))
</span><span class="cx"> storageGPR = baseForAccessGPR;
</span><span class="lines">@@ -986,6 +999,9 @@
</span><span class="cx"> CCallHelpers::tagFor(static_cast<VirtualRegister>(CallFrameSlot::argumentCount)));
</span><span class="cx">
</span><span class="cx"> if (m_type == Getter || m_type == Setter) {
</span><ins>+ ASSERT(baseGPR != loadedValueGPR);
+ ASSERT(m_type != Setter || (baseGPR != valueRegsPayloadGPR && loadedValueGPR != valueRegsPayloadGPR));
+
</ins><span class="cx"> // Create a JS call using a JS call inline cache. Assume that:
</span><span class="cx"> //
</span><span class="cx"> // - SP is aligned and represents the extent of the calling compiler's stack usage.
</span><span class="lines">@@ -1064,7 +1080,7 @@
</span><span class="cx"> loadedValueGPR, calleeFrame.withOffset(CallFrameSlot::callee * sizeof(Register)));
</span><span class="cx">
</span><span class="cx"> jit.storeCell(
</span><del>- baseForGetGPR,
</del><ins>+ baseGPR,
</ins><span class="cx"> calleeFrame.withOffset(virtualRegisterForArgument(0).offset() * sizeof(Register)));
</span><span class="cx">
</span><span class="cx"> if (m_type == Setter) {
</span><span class="lines">@@ -1118,6 +1134,8 @@
</span><span class="cx"> CodeLocationLabel(vm.getCTIStub(linkCallThunkGenerator).code()));
</span><span class="cx"> });
</span><span class="cx"> } else {
</span><ins>+ ASSERT(m_type == CustomValueGetter || m_type == CustomAccessorGetter || m_type == CustomValueSetter || m_type == CustomAccessorSetter);
+
</ins><span class="cx"> // Need to make room for the C call so any of our stack spillage isn't overwritten. It's
</span><span class="cx"> // hard to track if someone did spillage or not, so we just assume that we always need
</span><span class="cx"> // to make some space here.
</span></span></pre>
</div>
</div>
</body>
</html>