Bug 172673 – Assertion failure in com.apple.WebKit.WebContent.Development in com.apple.JavaScriptCore: JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined + 141

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryan Haddad]

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

[Ryan Haddad]

<rdar://problem/32250144>

[Ryan Haddad]

This assertion failure is happening very frequently on the bots.

[Saam Barati]

(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.

[Saam Barati]

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/

[Saam Barati]

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.)

[Saam Barati]

(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.)

[Saam Barati]

Created attachment 312161 [details]
patch

[Mark Lam]

Comment on attachment 312161 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 312161 [details]
patch

Clearing flags on attachment: 312161

Committed r217896: <http://trac.webkit.org/changeset/217896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

*** Bug 168587 has been marked as a duplicate of this bug. ***