WebKit Bugzilla
Attachment 341607 Details for
Bug 186112
: LLInt get_by_id prototype caching doesn't properly handle changes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-186112-20180530150921.patch (text/plain), 11.77 KB, created by
Keith Miller
on 2018-05-30 15:09:22 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Keith Miller
Created:
2018-05-30 15:09:22 PDT
Size:
11.77 KB
patch
obsolete
>Subversion Revision: 232138 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 28a319a8b2dbf4cc82616e79b811be97a013974b..3aff857c6905a1b628f52ad2dd7a28b377adb188 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,25 @@ >+2018-05-30 Keith Miller <keith_miller@apple.com> >+ >+ LLInt get_by_id prototype caching doesn't properly handle changes >+ https://bugs.webkit.org/show_bug.cgi?id=186112 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ The caching would sometimes fail to track that a prototype had changed >+ and wouldn't update its set of watchpoints. >+ >+ * bytecode/CodeBlock.cpp: >+ (JSC::CodeBlock::finalizeLLIntInlineCaches): >+ * bytecode/CodeBlock.h: >+ * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h: >+ (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::key const): >+ * bytecode/ObjectPropertyConditionSet.h: >+ (JSC::ObjectPropertyConditionSet::size const): >+ * bytecode/Watchpoint.h: >+ (JSC::Watchpoint::Watchpoint): Deleted. >+ * llint/LLIntSlowPaths.cpp: >+ (JSC::LLInt::setupGetByIdPrototypeCache): >+ > 2018-05-23 Keith Miller <keith_miller@apple.com> > > Define length on CoW array should properly convert to writable >diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog >index 7bae582c6b028421e0c674dd929ba70cafb7230a..58a014c90a7f3440998fb70acaf5fa16164f43a0 100644 >--- a/Source/WTF/ChangeLog >+++ b/Source/WTF/ChangeLog >@@ -1,3 +1,19 @@ >+2018-05-30 Keith Miller <keith_miller@apple.com> >+ >+ LLInt get_by_id prototype caching doesn't properly handle changes >+ https://bugs.webkit.org/show_bug.cgi?id=186112 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Mark some methods const. >+ >+ * wtf/Bag.h: >+ (WTF::Bag::begin const): >+ (WTF::Bag::end const): >+ (WTF::Bag::unwrappedHead const): >+ (WTF::Bag::end): Deleted. >+ (WTF::Bag::unwrappedHead): Deleted. >+ > 2018-05-23 Eric Carlson <eric.carlson@apple.com> > > Avoid loading AVFoundation to check supported MIME types if possible >diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp >index 8bd7fe7af83de12d9e19d2df40dbc3c474d1c37c..fc5783a799e79073326e2e3563d1368bda5315e0 100644 >--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp >+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp >@@ -1259,9 +1259,7 @@ void CodeBlock::finalizeLLIntInlineCaches() > for (size_t size = propertyAccessInstructions.size(), i = 0; i < size; ++i) { > Instruction* curInstruction = &instructions()[propertyAccessInstructions[i]]; > switch (Interpreter::getOpcodeID(curInstruction[0])) { >- case op_get_by_id: >- case op_get_by_id_proto_load: >- case op_get_by_id_unset: { >+ case op_get_by_id: { > StructureID oldStructureID = curInstruction[4].u.structureID; > if (!oldStructureID || Heap::isMarked(vm.heap.structureIDTable().get(oldStructureID))) > break; >@@ -1300,6 +1298,8 @@ void CodeBlock::finalizeLLIntInlineCaches() > // We need to add optimizations for op_resolve_scope_for_hoisting_func_decl_in_eval to do link time scope resolution. > case op_resolve_scope_for_hoisting_func_decl_in_eval: > break; >+ case op_get_by_id_proto_load: >+ case op_get_by_id_unset: > case op_get_array_length: > break; > case op_to_this: >@@ -1357,8 +1357,27 @@ void CodeBlock::finalizeLLIntInlineCaches() > > // We can't just remove all the sets when we clear the caches since we might have created a watchpoint set > // then cleared the cache without GCing in between. >- m_llintGetByIdWatchpointMap.removeIf([](const StructureWatchpointMap::KeyValuePairType& pair) -> bool { >- return !Heap::isMarked(pair.key); >+ m_llintGetByIdWatchpointMap.removeIf([&] (const StructureWatchpointMap::KeyValuePairType& pair) -> bool { >+ auto clear = [&] () { >+ Instruction* instruction = std::get<1>(pair.key); >+ OpcodeID opcode = Interpreter::getOpcodeID(*instruction); >+ if (opcode == op_get_by_id_proto_load || opcode == op_get_by_id_unset) { >+ if (Options::verboseOSR()) >+ dataLogF("Clearing LLInt property access.\n"); >+ clearLLIntGetByIdCache(instruction); >+ } >+ return true; >+ }; >+ >+ if (!Heap::isMarked(std::get<0>(pair.key))) >+ return clear(); >+ >+ for (const LLIntPrototypeLoadAdaptiveStructureWatchpoint* watchpoint : pair.value) { >+ if (!watchpoint->key().isStillLive()) >+ return clear(); >+ } >+ >+ return false; > }); > > for (unsigned i = 0; i < m_llintCallLinkInfos.size(); ++i) { >diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h >index fded8ea4b65cb05f1484d0996cc4d889b6bf13a2..0d4ed3af372ed46164d28916a836d385a2622a71 100644 >--- a/Source/JavaScriptCore/bytecode/CodeBlock.h >+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h >@@ -627,7 +627,7 @@ public: > return m_llintExecuteCounter; > } > >- typedef HashMap<Structure*, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap; >+ typedef HashMap<std::tuple<Structure*, Instruction*>, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap; > StructureWatchpointMap& llintGetByIdWatchpointMap() { return m_llintGetByIdWatchpointMap; } > > // Functions for controlling when tiered compilation kicks in. This >diff --git a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h >index 8a73c6c79d7fd354658ec6a39caa40d1a043058e..66468f0ef845bc3b7f6549d0eddb35038d358eb0 100644 >--- a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h >+++ b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h >@@ -33,16 +33,19 @@ namespace JSC { > > class LLIntPrototypeLoadAdaptiveStructureWatchpoint : public Watchpoint { > public: >+ LLIntPrototypeLoadAdaptiveStructureWatchpoint() = default; > LLIntPrototypeLoadAdaptiveStructureWatchpoint(const ObjectPropertyCondition&, Instruction*); > > void install(); > >+ const ObjectPropertyCondition& key() const { return m_key; } >+ > protected: > void fireInternal(const FireDetail&) override; > > private: > ObjectPropertyCondition m_key; >- Instruction* m_getByIdInstruction; >+ Instruction* m_getByIdInstruction { nullptr }; > }; > > } // namespace JSC >diff --git a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h >index 493e20c65211c2e0bf5f02ced21b9f73c1b76706..8e53a1b27b4723b314acc17055c78d14ad193176 100644 >--- a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h >+++ b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h >@@ -67,7 +67,8 @@ public: > } > > bool isValidAndWatchable() const; >- >+ >+ size_t size() const { return m_data ? m_data->vector.size() : 0; } > bool isEmpty() const > { > return !m_data; >diff --git a/Source/JavaScriptCore/bytecode/Watchpoint.h b/Source/JavaScriptCore/bytecode/Watchpoint.h >index 4b373496fb97a70893284a02fe468deacd192888..4476e2a7b5135ea7dbcf2ec688c7397b72900e01 100644 >--- a/Source/JavaScriptCore/bytecode/Watchpoint.h >+++ b/Source/JavaScriptCore/bytecode/Watchpoint.h >@@ -68,9 +68,7 @@ class Watchpoint : public BasicRawSentinelNode<Watchpoint> { > WTF_MAKE_NONCOPYABLE(Watchpoint); > WTF_MAKE_FAST_ALLOCATED; > public: >- Watchpoint() >- { >- } >+ Watchpoint() = default; > > virtual ~Watchpoint(); > >diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp >index 5ccdcfe4146496b9aebe51010a977ca276e1da68..21212587296c57feb50617e71e84db949030ea9e 100644 >--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp >+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp >@@ -670,15 +670,18 @@ static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, Instruction* pc, > > PropertyOffset offset = invalidOffset; > CodeBlock::StructureWatchpointMap& watchpointMap = codeBlock->llintGetByIdWatchpointMap(); >- auto result = watchpointMap.add(structure, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>()); >+ Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint> watchpoints; > for (ObjectPropertyCondition condition : conditions) { > if (!condition.isWatchable()) > return; > if (condition.condition().kind() == PropertyCondition::Presence) > offset = condition.condition().offset(); >- result.iterator->value.add(condition, pc)->install(); >+ watchpoints.add(condition, pc)->install(); > } >+ > ASSERT((offset == invalidOffset) == slot.isUnset()); >+ auto result = watchpointMap.add(std::make_tuple(structure, pc), WTFMove(watchpoints)); >+ ASSERT_UNUSED(result, result.isNewEntry); > > ConcurrentJSLocker locker(codeBlock->m_lock); > >diff --git a/Source/WTF/wtf/Bag.h b/Source/WTF/wtf/Bag.h >index feafbaa6eccc19ff6efe7d9876b7e85928eddc87..f66eed38d18cba59fb4b76f02ccf1c23e4571f46 100644 >--- a/Source/WTF/wtf/Bag.h >+++ b/Source/WTF/wtf/Bag.h >@@ -138,13 +138,21 @@ public: > result.m_node = unwrappedHead(); > return result; > } >- >- iterator end() { return iterator(); } >+ >+ const iterator begin() const >+ { >+ iterator result; >+ result.m_node = unwrappedHead(); >+ return result; >+ } >+ >+ >+ iterator end() const { return iterator(); } > > bool isEmpty() const { return !m_head; } > > private: >- Node* unwrappedHead() { return PtrTraits::unwrap(m_head); } >+ Node* unwrappedHead() const { return PtrTraits::unwrap(m_head); } > > typename PtrTraits::StorageType m_head { nullptr }; > }; >diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog >index 645793478a2711adddb7914f5a4a37b634c4ec0f..4ecd7927e07169454e797f3948beb1d45d2aa20e 100644 >--- a/JSTests/ChangeLog >+++ b/JSTests/ChangeLog >@@ -1,3 +1,15 @@ >+2018-05-30 Keith Miller <keith_miller@apple.com> >+ >+ LLInt get_by_id prototype caching doesn't properly handle changes >+ https://bugs.webkit.org/show_bug.cgi?id=186112 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * stress/llint-proto-get-by-id-cache-change-prototype.js: Added. >+ (foo): >+ * stress/llint-proto-get-by-id-cache-intercept-value.js: Added. >+ (foo): >+ > 2018-05-23 Keith Miller <keith_miller@apple.com> > > Define length on CoW array should properly convert to writable >diff --git a/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js b/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js >new file mode 100644 >index 0000000000000000000000000000000000000000..4f70b03f55bfcf7f670d388edad58cc124a16282 >--- /dev/null >+++ b/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js >@@ -0,0 +1,19 @@ >+let p = Object.create({ foo: 1 }); >+let o = Object.create(p); >+ >+let other = { foo: 10 }; >+ >+function foo() { >+ return o.foo >+} >+ >+for (let i = 0; i < 10; i++) >+ foo(); >+ >+p.__proto__ = null; >+gc(); >+ >+Object.defineProperty(other, "foo", { get() { } }); >+ >+if (foo() !== undefined) >+ throw new Error("bad get by id access"); >diff --git a/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js b/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js >new file mode 100644 >index 0000000000000000000000000000000000000000..0b90bea4d32a21e2e1ed82d764d4b4d9049e46d1 >--- /dev/null >+++ b/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js >@@ -0,0 +1,17 @@ >+let p = Object.create({ foo: 1 }); >+let o = Object.create(p); >+ >+let other = { foo: 10 }; >+ >+function foo() { >+ return o.foo >+} >+ >+for (let i = 0; i < 10; i++) >+ foo(); >+ >+p.foo = null; >+gc(); >+ >+if (foo() !== null) >+ throw new Error("bad get by id access");
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 186112
: 341607