<!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>[191134] trunk/Source/JavaScriptCore</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/191134">191134</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-10-15 13:34:10 -0700 (Thu, 15 Oct 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>InferredTypeTable should ref its keys
https://bugs.webkit.org/show_bug.cgi?id=150138
rdar://problem/23080555
Reviewed by Michael Saboff.
InferredTypeTable was incorrectly using a key hash traits that caused the underlying HashTable to
store keys as UniquedStringImpl* rather than RefPtr<UniquedStringImpl>, even though the HashMap's
nominal key type was RefPtr<UniquedStringImpl>. This arose because I copy-pasted the HashMap type
instantiation from other places and then made random changes to adapt it to my needs, rather than
actually thinking about what I was doing. The solution is to remove the key hash traits argument,
since all it accomplishes is to produce this bug.
The way this bug manifested is probably best described in http://webkit.org/b/150008. After a while
the InferredTypeTable would have dangling references to its strings, if some recompilation or other
thing caused us to drop all other references to those strings. InferredTypeTable is particularly
susceptible to this because it is designed to know about a superset of the property names that its
client Structures know about. The debug assert would then happen when we rehashed the
InferredTypeTable's HashMap, because we'd try to get the hashes of strings that were already
deleted. AFAICT, we didn't have release crashes arising from those strings' memory being returned
to the OS - but it's totally possible that this could have happened. So, we definitely should treat
this bug as more than just a debug issue.
Interestingly, we could have also solved this problem by changing the hash function to use PtrHash.
In all other ways, it's OK for InferredTypeTable to hold dangling references, since it uses the
address of the UniquedStringImpl as a way to name an abstract heap. It's fine if the name of an
abstract heap is a bogus memory address, and it's also fine if that name referred to an entirely
different UniquedStringImpl at some point in the past. That's a nice benefit of any data structure
that keys by abstract heap - if two of them get unified then it's no big deal. I've filed another
bug, http://webkit.org/b/150137 about changing all of our UniquedStringImpl* hashing to use
PtrHash.
* runtime/Identifier.h: Add a comment about http://webkit.org/b/150137.
* runtime/InferredTypeTable.h: Fix the bug.
* tests/stress/inferred-type-table-stale-identifiers.js: Added. I couldn't get this to cause a crash before my change, but it's an interesting test nonetheless.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeIdentifierh">trunk/Source/JavaScriptCore/runtime/Identifier.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeInferredTypeTableh">trunk/Source/JavaScriptCore/runtime/InferredTypeTable.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstressinferredtypetablestaleidentifiersjs">trunk/Source/JavaScriptCore/tests/stress/inferred-type-table-stale-identifiers.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (191133 => 191134)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-10-15 20:21:06 UTC (rev 191133)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-10-15 20:34:10 UTC (rev 191134)
</span><span class="lines">@@ -1,3 +1,41 @@
</span><ins>+2015-10-15 Filip Pizlo <fpizlo@apple.com>
+
+ InferredTypeTable should ref its keys
+ https://bugs.webkit.org/show_bug.cgi?id=150138
+ rdar://problem/23080555
+
+ Reviewed by Michael Saboff.
+
+ InferredTypeTable was incorrectly using a key hash traits that caused the underlying HashTable to
+ store keys as UniquedStringImpl* rather than RefPtr<UniquedStringImpl>, even though the HashMap's
+ nominal key type was RefPtr<UniquedStringImpl>. This arose because I copy-pasted the HashMap type
+ instantiation from other places and then made random changes to adapt it to my needs, rather than
+ actually thinking about what I was doing. The solution is to remove the key hash traits argument,
+ since all it accomplishes is to produce this bug.
+
+ The way this bug manifested is probably best described in http://webkit.org/b/150008. After a while
+ the InferredTypeTable would have dangling references to its strings, if some recompilation or other
+ thing caused us to drop all other references to those strings. InferredTypeTable is particularly
+ susceptible to this because it is designed to know about a superset of the property names that its
+ client Structures know about. The debug assert would then happen when we rehashed the
+ InferredTypeTable's HashMap, because we'd try to get the hashes of strings that were already
+ deleted. AFAICT, we didn't have release crashes arising from those strings' memory being returned
+ to the OS - but it's totally possible that this could have happened. So, we definitely should treat
+ this bug as more than just a debug issue.
+
+ Interestingly, we could have also solved this problem by changing the hash function to use PtrHash.
+ In all other ways, it's OK for InferredTypeTable to hold dangling references, since it uses the
+ address of the UniquedStringImpl as a way to name an abstract heap. It's fine if the name of an
+ abstract heap is a bogus memory address, and it's also fine if that name referred to an entirely
+ different UniquedStringImpl at some point in the past. That's a nice benefit of any data structure
+ that keys by abstract heap - if two of them get unified then it's no big deal. I've filed another
+ bug, http://webkit.org/b/150137 about changing all of our UniquedStringImpl* hashing to use
+ PtrHash.
+
+ * runtime/Identifier.h: Add a comment about http://webkit.org/b/150137.
+ * runtime/InferredTypeTable.h: Fix the bug.
+ * tests/stress/inferred-type-table-stale-identifiers.js: Added. I couldn't get this to cause a crash before my change, but it's an interesting test nonetheless.
+
</ins><span class="cx"> 2015-10-15 Mark Lam <mark.lam@apple.com>
</span><span class="cx">
</span><span class="cx"> Add MASM_PROBE support for ARM64.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeIdentifierh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/Identifier.h (191133 => 191134)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/Identifier.h 2015-10-15 20:21:06 UTC (rev 191133)
+++ trunk/Source/JavaScriptCore/runtime/Identifier.h 2015-10-15 20:34:10 UTC (rev 191134)
</span><span class="lines">@@ -282,6 +282,10 @@
</span><span class="cx"> return parseIndex(*uid);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+// FIXME: It may be better for this to just be a typedef for PtrHash, since PtrHash may be cheaper to
+// compute than loading the StringImpl's hash from memory. That change would also reduce the likelihood of
+// crashes in code that somehow dangled a StringImpl.
+// https://bugs.webkit.org/show_bug.cgi?id=150137
</ins><span class="cx"> struct IdentifierRepHash : PtrHash<RefPtr<UniquedStringImpl>> {
</span><span class="cx"> static unsigned hash(const RefPtr<UniquedStringImpl>& key) { return key->existingSymbolAwareHash(); }
</span><span class="cx"> static unsigned hash(UniquedStringImpl* key) { return key->existingSymbolAwareHash(); }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeInferredTypeTableh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/InferredTypeTable.h (191133 => 191134)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/InferredTypeTable.h 2015-10-15 20:21:06 UTC (rev 191133)
+++ trunk/Source/JavaScriptCore/runtime/InferredTypeTable.h 2015-10-15 20:34:10 UTC (rev 191134)
</span><span class="lines">@@ -98,7 +98,7 @@
</span><span class="cx"> // that's bad. We avoid such confusion by ensuring that a transition always adds an entry. Hence,
</span><span class="cx"> // absence-means-bottom only comes into play for properties added before the InferredTypeTable was
</span><span class="cx"> // created.
</span><del>- typedef HashMap<RefPtr<UniquedStringImpl>, WriteBarrier<InferredType>, IdentifierRepHash, HashTraits<UniquedStringImpl*>> TableType;
</del><ins>+ typedef HashMap<RefPtr<UniquedStringImpl>, WriteBarrier<InferredType>, IdentifierRepHash> TableType;
</ins><span class="cx">
</span><span class="cx"> TableType m_table;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressinferredtypetablestaleidentifiersjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/inferred-type-table-stale-identifiers.js (0 => 191134)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/inferred-type-table-stale-identifiers.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/inferred-type-table-stale-identifiers.js 2015-10-15 20:34:10 UTC (rev 191134)
</span><span class="lines">@@ -0,0 +1,220 @@
</span><ins>+var o = {};
+o.f = 1;
+o.g = 2;
+
+var p = {};
+p.f = 1;
+p.g = 2;
+
+function foo(o) {
+ o.i0 = 0
+ o.i1 = 1
+ o.i2 = 2
+ o.i3 = 3
+ o.i4 = 4
+ o.i5 = 5
+ o.i6 = 6
+ o.i7 = 7
+ o.i8 = 8
+ o.i9 = 9
+ o.i10 = 10
+ o.i11 = 11
+ o.i12 = 12
+ o.i13 = 13
+ o.i14 = 14
+ o.i15 = 15
+ o.i16 = 16
+ o.i17 = 17
+ o.i18 = 18
+ o.i19 = 19
+ o.i20 = 20
+ o.i21 = 21
+ o.i22 = 22
+ o.i23 = 23
+ o.i24 = 24
+ o.i25 = 25
+ o.i26 = 26
+ o.i27 = 27
+ o.i28 = 28
+ o.i29 = 29
+ o.i30 = 30
+ o.i31 = 31
+ o.i32 = 32
+ o.i33 = 33
+ o.i34 = 34
+ o.i35 = 35
+ o.i36 = 36
+ o.i37 = 37
+ o.i38 = 38
+ o.i39 = 39
+ o.i40 = 40
+ o.i41 = 41
+ o.i42 = 42
+ o.i43 = 43
+ o.i44 = 44
+ o.i45 = 45
+ o.i46 = 46
+ o.i47 = 47
+ o.i48 = 48
+ o.i49 = 49
+ o.i50 = 50
+ o.i51 = 51
+ o.i52 = 52
+ o.i53 = 53
+ o.i54 = 54
+ o.i55 = 55
+ o.i56 = 56
+ o.i57 = 57
+ o.i58 = 58
+ o.i59 = 59
+ o.i60 = 60
+ o.i61 = 61
+ o.i62 = 62
+ o.i63 = 63
+ o.i64 = 64
+ o.i65 = 65
+ o.i66 = 66
+ o.i67 = 67
+ o.i68 = 68
+ o.i69 = 69
+ o.i70 = 70
+ o.i71 = 71
+ o.i72 = 72
+ o.i73 = 73
+ o.i74 = 74
+ o.i75 = 75
+ o.i76 = 76
+ o.i77 = 77
+ o.i78 = 78
+ o.i79 = 79
+ o.i80 = 80
+ o.i81 = 81
+ o.i82 = 82
+ o.i83 = 83
+ o.i84 = 84
+ o.i85 = 85
+ o.i86 = 86
+ o.i87 = 87
+ o.i88 = 88
+ o.i89 = 89
+ o.i90 = 90
+ o.i91 = 91
+ o.i92 = 92
+ o.i93 = 93
+ o.i94 = 94
+ o.i95 = 95
+ o.i96 = 96
+ o.i97 = 97
+ o.i98 = 98
+ o.i99 = 99
+}
+
+foo(o);
+foo = null;
+o = null;
+fullGC();
+
+function bar(o) {
+ o.j0 = 0
+ o.j1 = 1
+ o.j2 = 2
+ o.j3 = 3
+ o.j4 = 4
+ o.j5 = 5
+ o.j6 = 6
+ o.j7 = 7
+ o.j8 = 8
+ o.j9 = 9
+ o.j10 = 10
+ o.j11 = 11
+ o.j12 = 12
+ o.j13 = 13
+ o.j14 = 14
+ o.j15 = 15
+ o.j16 = 16
+ o.j17 = 17
+ o.j18 = 18
+ o.j19 = 19
+ o.j20 = 20
+ o.j21 = 21
+ o.j22 = 22
+ o.j23 = 23
+ o.j24 = 24
+ o.j25 = 25
+ o.j26 = 26
+ o.j27 = 27
+ o.j28 = 28
+ o.j29 = 29
+ o.j30 = 30
+ o.j31 = 31
+ o.j32 = 32
+ o.j33 = 33
+ o.j34 = 34
+ o.j35 = 35
+ o.j36 = 36
+ o.j37 = 37
+ o.j38 = 38
+ o.j39 = 39
+ o.j40 = 40
+ o.j41 = 41
+ o.j42 = 42
+ o.j43 = 43
+ o.j44 = 44
+ o.j45 = 45
+ o.j46 = 46
+ o.j47 = 47
+ o.j48 = 48
+ o.j49 = 49
+ o.j50 = 50
+ o.j51 = 51
+ o.j52 = 52
+ o.j53 = 53
+ o.j54 = 54
+ o.j55 = 55
+ o.j56 = 56
+ o.j57 = 57
+ o.j58 = 58
+ o.j59 = 59
+ o.j60 = 60
+ o.j61 = 61
+ o.j62 = 62
+ o.j63 = 63
+ o.j64 = 64
+ o.j65 = 65
+ o.j66 = 66
+ o.j67 = 67
+ o.j68 = 68
+ o.j69 = 69
+ o.j70 = 70
+ o.j71 = 71
+ o.j72 = 72
+ o.j73 = 73
+ o.j74 = 74
+ o.j75 = 75
+ o.j76 = 76
+ o.j77 = 77
+ o.j78 = 78
+ o.j79 = 79
+ o.j80 = 80
+ o.j81 = 81
+ o.j82 = 82
+ o.j83 = 83
+ o.j84 = 84
+ o.j85 = 85
+ o.j86 = 86
+ o.j87 = 87
+ o.j88 = 88
+ o.j89 = 89
+ o.j90 = 90
+ o.j91 = 91
+ o.j92 = 92
+ o.j93 = 93
+ o.j94 = 94
+ o.j95 = 95
+ o.j96 = 96
+ o.j97 = 97
+ o.j98 = 98
+ o.j99 = 99
+}
+
+bar(p);
</ins></span></pre>
</div>
</div>
</body>
</html>