Bug 182494 – Avoid unnecessarily constructing RunLoops for GC AutomaticThreads in Connection::sendMessage() after r228001

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Chris Dumez]

Thread[20] EXC_BREAKPOINT (SIGTRAP) (0x0000000000000002, 0x0000000000000000)
[  0] 0x0000000104a6f1a3 JavaScriptCore`WTF::RunLoop::current() + 163

     0x0000000104a6f19f:     popq %rbp
     0x0000000104a6f1a0:     retq 
     0x0000000104a6f1a1:     int3 
     0x0000000104a6f1a2:     int3 
 ->  0x0000000104a6f1a3:     nopw %cs:(%rax,%rax)

[  1] 0x0000000104a6f1d1 JavaScriptCore`WTF::RunLoop::isMain() + 17 at RunLoop.cpp:74:30
[  2] 0x00000001005ece7c WebKit`IPC::Connection::sendMessage(std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >, WTF::OptionSet<IPC::SendOption>) + 44 at Connection.cpp:382:9
[  3] 0x00000001007abc96 WebKit`bool IPC::Connection::send<Messages::WebProcessProxy::CheckRemotePortForActivity>(Messages::WebProcessProxy::CheckRemotePortForActivity&&, unsigned long long, WTF::OptionSet<IPC::SendOption>) + 106 at Connection.h:361:12
[  4] 0x00000001007abc08 WebKit`WebKit::WebMessagePortChannelProvider::checkRemotePortForActivity(WebCore::MessagePortIdentifier const&, WTF::CompletionHandler<void (WebCore::MessagePortChannelProvider::HasActivity)>&&) + 460 at WebMessagePortChannelProvider.cpp:146:56
[  5] 0x0000000101f051d5 WebCore`WebCore::MessagePort::hasPendingActivity() const + 149 at MessagePort.cpp:304:49
[  6] 0x00000001012b36cc WebCore`WebCore::JSMessagePortOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&) + 28 at JSMessagePort.cpp:270:34
[  7] 0x00000001045eb7bb JavaScriptCore`void JSC::WeakBlock::specializedVisit<JSC::MarkedBlock>(JSC::MarkedBlock&, JSC::SlotVisitor&) + 123 at WeakBlock.cpp:117:31
[  8] 0x00000001045df88a JavaScriptCore`JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&) [inlined] JSC::WeakSet::visit(JSC::SlotVisitor&) + 18 at WeakSet.h:113:16
[  8] 0x00000001045df878 JavaScriptCore`JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&) [inlined] JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10::operator()(JSC::WeakSet*) const at MarkedSpace.cpp:278
[  8] 0x00000001045df878 JavaScriptCore`JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&) [inlined] void WTF::SentinelLinkedList<JSC::WeakSet, WTF::BasicRawSentinelNode<JSC::WeakSet> >::forEach<JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10>(JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10 const&) + 36 at SentinelLinkedList.h:110
[  8] 0x00000001045df854 JavaScriptCore`JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&) + 20 at MarkedSpace.cpp:281
[  9] 0x00000001045e1fdd JavaScriptCore`JSC::MarkingConstraintSolver::runExecutionThread(JSC::SlotVisitor&, JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<std::optional<unsigned int> ()>) [inlined] JSC::MarkingConstraint::execute(JSC::SlotVisitor&) + 16 at MarkingConstraint.cpp:57:5
[  9] 0x00000001045e1fcd JavaScriptCore`JSC::MarkingConstraintSolver::runExecutionThread(JSC::SlotVisitor&, JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<std::optional<unsigned int> ()>) + 1053 at MarkingConstraintSolver.cpp:238
[ 10] 0x00000001045e7fd3 JavaScriptCore`JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode, WTF::MonotonicTime) + 883 at SlotVisitor.cpp:657:24
[ 11] 0x00000001045cc67b JavaScriptCore`WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_14>::run() [inlined] JSC::Heap::runBeginPhase(JSC::GCConductor)::$_14::operator()() const + 136 at Heap.cpp:1260:30
[ 11] 0x00000001045cc5f3 JavaScriptCore`WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_14>::run() + 19 at SharedTask.h:92
[ 12] 0x0000000104a6a54b JavaScriptCore`WTF::ParallelHelperClient::runTask(WTF::RefPtr<WTF::SharedTask<void ()>, WTF::DumbPtrTraits<WTF::SharedTask<void ()> > >) + 43 at ParallelHelperPool.cpp:112:11
[ 13] 0x0000000104a6afbf JavaScriptCore`WTF::ParallelHelperPool::Thread::work() + 47 at ParallelHelperPool.cpp:194:19
[ 14] 0x0000000104a514d7 JavaScriptCore`WTF::Function<void ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>::call() [inlined] WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const + 274 at AutomaticThread.cpp:222:37
[ 14] 0x0000000104a513c5 JavaScriptCore`WTF::Function<void ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>::call() + 21 at Function.h:101
[ 15] 0x0000000104a7cc83 JavaScriptCore`WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) [inlined] WTF::Function<void ()>::operator()() const + 8 at Function.h:56:35
[ 15] 0x0000000104a7cc7b JavaScriptCore`WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 219 at Threading.cpp:129
[ 16] 0x0000000103e7ebe8 JavaScriptCore`WTF::wtfThreadEntryPoint(void*) + 8 at ThreadingPthreads.cpp:223:5
[ 17] 0x00007fff794866c0 libsystem_pthread.dylib`_pthread_body + 339 at pthread.c:740:17
[ 18] 0x00007fff7948656c libsystem_pthread.dylib`_pthread_start + 376 at pthread.c:799:2
[ 19] 0x00007fff79485c5c libsystem_pthread.dylib`thread_start + 12

[Chris Dumez]

<rdar://problem/37147632>

[Chris Dumez]

Created attachment 333092 [details]
Patch

Comment on attachment 333092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=333092&action=review

> Source/WebKit/ChangeLog:24
> +        Looking at the code, the most likely one is when constructing the ThreadSpecific:
> +        """
> +        int error = pthread_key_create(&m_key, destroy);
> +        if (error)
> +            CRASH();
> +        """

I don’t think that’s correct. When I hit this crash, it looked like what was failing was RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread()) in the ThreadSpecific constructor.

[Chris Dumez]

(In reply to mitz from comment #3)
> Comment on attachment 333092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333092&action=review
> 
> > Source/WebKit/ChangeLog:24
> > +        Looking at the code, the most likely one is when constructing the ThreadSpecific:
> > +        """
> > +        int error = pthread_key_create(&m_key, destroy);
> > +        if (error)
> > +            CRASH();
> > +        """
> 
> I don’t think that’s correct. When I hit this crash, it looked like what was
> failing was RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True ||
> !mayBeGCThread()) in the ThreadSpecific constructor.

That is interesting. RunLoop::current() is implemented like so:
RunLoop& RunLoop::current()
{
    static NeverDestroyed<ThreadSpecific<Holder, CanBeGCThread::True>> runLoopHolder;
    return runLoopHolder.get()->runLoop();
}

So this assertion should have been disabled due to CanBeGCThread::True. Either way, switching to isMainThread() will bypass this code path entirely.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to mitz from comment #3)
> > Comment on attachment 333092 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333092&action=review
> > 
> > > Source/WebKit/ChangeLog:24
> > > +        Looking at the code, the most likely one is when constructing the ThreadSpecific:
> > > +        """
> > > +        int error = pthread_key_create(&m_key, destroy);
> > > +        if (error)
> > > +            CRASH();
> > > +        """
> > 
> > I don’t think that’s correct. When I hit this crash, it looked like what was
> > failing was RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True ||
> > !mayBeGCThread()) in the ThreadSpecific constructor.
> 
> That is interesting. RunLoop::current() is implemented like so:
> RunLoop& RunLoop::current()
> {
>     static NeverDestroyed<ThreadSpecific<Holder, CanBeGCThread::True>>
> runLoopHolder;
>     return runLoopHolder.get()->runLoop();
> }
> 
> So this assertion should have been disabled due to CanBeGCThread::True.
> Either way, switching to isMainThread() will bypass this code path entirely.

Found it, somebody made this change very recently:
https://bugs.webkit.org/show_bug.cgi?id=182104

Previously, it was not allowed to call this method from a GC thread.

[Chris Dumez]

Created attachment 333096 [details]
Patch

So this crash is a duplicate of bug 182104, and is already fixed. Perhaps the idea of changing Connection::sendMessage() should be tracked separately (or this bug retitled and separated from the Radar).

[Chris Dumez]

(In reply to mitz from comment #7)
> So this crash is a duplicate of bug 182104, and is already fixed. Perhaps
> the idea of changing Connection::sendMessage() should be tracked separately
> (or this bug retitled and separated from the Radar).

Well, there is "fixed" and "fixed". We keep constructing RunLoops for AutomaticThreads for no reason now.

[Chris Dumez]

Created attachment 333097 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/37242864>

[EWS Watchlist]

Comment on attachment 333097 [details]
Patch

Attachment 333097 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6372965

New failing tests:
fast/scrolling/scroll-to-focused-element-asynchronously.html

[EWS Watchlist]

Created attachment 333132 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 333097 [details]
Patch

Clearing flags on attachment: 333097

Committed r228152: <https://trac.webkit.org/changeset/228152>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.