This assertion failure is seen with imported/w3c/web-platform-tests/streams/readable-streams/general.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r217506%20(1488)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fstreams%2Freadable-streams%2Fgeneral.html ASSERTION FAILED: The Compare should have been eliminated, it is known to be always false. !masqueradesAsUndefinedWatchpointIsStillValid() || !isKnownCell(operand.node()) /Volumes/Data/slave/elcapitan-debug/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp(305) : void JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined(JSC::DFG::Edge, JSC::DFG::Node *) 1 0x103817ae0 WTFCrash 2 0x102d7c53d JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined(JSC::DFG::Edge, JSC::DFG::Node*) 3 0x102cf4b26 JSC::DFG::SpeculativeJIT::compilePeepHoleBranch(JSC::DFG::Node*, JSC::MacroAssemblerX86Common::RelationalCondition, JSC::MacroAssemblerX86Common::DoubleCondition, unsigned long (*)(JSC::ExecState*, long long, long long)) 4 0x102d082d6 JSC::DFG::SpeculativeJIT::compare(JSC::DFG::Node*, JSC::MacroAssemblerX86Common::RelationalCondition, JSC::MacroAssemblerX86Common::DoubleCondition, unsigned long (*)(JSC::ExecState*, long long, long long)) 5 0x102d89260 JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*) 6 0x102cf5603 JSC::DFG::SpeculativeJIT::compileCurrentBlock() 7 0x102cf5dd6 JSC::DFG::SpeculativeJIT::compile() 8 0x102ba9ff7 JSC::DFG::JITCompiler::compileBody() 9 0x102bade25 JSC::DFG::JITCompiler::compileFunction() 10 0x102ca7155 JSC::DFG::Plan::compileInThreadImpl() 11 0x102ca445d JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*) 12 0x102e2b69e JSC::DFG::Worklist::ThreadBody::work() 13 0x10382544e WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const 14 0x1038251dd void std::__1::__invoke_void_return_wrapper<void>::__call<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&>(WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&&&) 15 0x103824fcc std::__1::__function::__func<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, std::__1::allocator<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>, void ()>::operator()() 16 0x102d2608a std::__1::function<void ()>::operator()() const 17 0x10389c4ae WTF::threadEntryPoint(void*) 18 0x10389e636 WTF::wtfThreadEntryPoint(void*) 19 0x7fff8bde499d _pthread_body 20 0x7fff8bde491a _pthread_body 21 0x7fff8bde2351 thread_start
<rdar://problem/32250144>
This assertion failure is happening very frequently on the bots.
(In reply to Ryan Haddad from comment #2) > This assertion failure is happening very frequently on the bots. I'll try to look into why this is happening.
This repros easily for me locally: ./Tools/Scripts/run-webkit-tests --repeat-each=1000 --fully-parallel --child-processes 10 --no-retry-failures LayoutTests/im/readable-streams/general.htmlstreams/
I figured out the bug, it's a race condition. Consider a program with: - a DFGFrozenValue over an object O and Structure S1. S1 starts off as dfgWatchable() being true - Structure S2 The program is like this: ``` a: JSConstant(O) b: CheckStructure(@a, S2) c: ToThis(@a) d: CheckEq(@c, nullConstant) ``` The AbstractValue for @a will start off as having a finite structure. When running AI, we'll notice that node @b will OSR exit, so nodes after @b are unreachable. Later in the compilation, S1 is no longer dfgWatchable(). Now, when running AI, @a will have Top for its structure set. No longer will we think @b exits. The DFG backend asserts that under such a situation, we should have simplified the CheckEq to false. However, this is a racy thing to assert (as demonstrated above). We should probably remove this assertion since it's always ok to not perform an optimization like this. However, we could also extend AbstractValue::set(DFGFrozenValue) to always be idempotent. It's slightly unexpected that the resulting AbstractValue's state will depend on a race with the main thread. We could make such an operation idempotent if we added a bit to RegisteredStructure that indicates if it's watched or not. And then we'd mandate all FrozenValues that are cells have Structure's that are watched. Perhaps this is overkill, but maybe it'd weed out other bugs. There is also something to be said for that it's good to know about such bugs that are caused by races like this. (Note, this program will never see the light of day. Since we noticed S1 as dfgWatchable(), we make the compilation dependent on S1 not transitioning. S1 transitions, so we won't actually run the code that gets compiled.)
(In reply to Saam Barati from comment #5) > I figured out the bug, it's a race condition. > > Consider a program with: > - a DFGFrozenValue over an object O and Structure S1. S1 starts off as > dfgWatchable() being true > - Structure S2 > > The program is like this: > > ``` > a: JSConstant(O) > b: CheckStructure(@a, S2) > c: ToThis(@a) > d: CheckEq(@c, nullConstant) > ``` > > The AbstractValue for @a will start off as having a finite structure. When > running AI, we'll notice that node @b will OSR exit, so nodes after @b are > unreachable. Later in the compilation, S1 is no longer dfgWatchable(). Now, > when running AI, @a will have Top for its structure set. No longer will we > think @b exits. > > The DFG backend asserts that under such a situation, we should have > simplified the CheckEq to false. However, this is a racy thing to assert (as > demonstrated above). We should probably remove this assertion since it's > always ok to not perform an optimization like this. > > However, we could also extend AbstractValue::set(DFGFrozenValue) to always > be idempotent. It's slightly unexpected that the resulting AbstractValue's > state will depend on a race with the main thread. We could make such an > operation idempotent if we added a bit to RegisteredStructure that indicates > if it's watched or not. And then we'd mandate all FrozenValues that are > cells have Structure's that are watched. Perhaps this is overkill, but maybe > it'd weed out other bugs. There is also something to be said for that it's > good to know about such bugs that are caused by races like this. If we think this is a good idea (it might be just because it'll save us a hash table lookup with constants), then we can do it in another patch. I think we should remove this assertion regardless. > > (Note, this program will never see the light of day. Since we noticed S1 as > dfgWatchable(), we make the compilation dependent on S1 not transitioning. > S1 transitions, so we won't actually run the code that gets compiled.)
Created attachment 312161 [details] patch
Comment on attachment 312161 [details] patch r=me
Comment on attachment 312161 [details] patch Clearing flags on attachment: 312161 Committed r217896: <http://trac.webkit.org/changeset/217896>
All reviewed patches have been landed. Closing bug.
*** Bug 168587 has been marked as a duplicate of this bug. ***