| Summary: | Avoid unnecessarily constructing RunLoops for GC AutomaticThreads in Connection::sendMessage() after r228001 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
| Component: | Service Workers | Assignee: | Chris Dumez <cdumez> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | beidson, commit-queue, ews-watchlist, fpizlo, ggaren, Hironori.Fujii, mitz, rniwa, youennf | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=182104 | ||||||||||||
| Attachments: |
|
||||||||||||
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. (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. (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. 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). (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. Created attachment 333097 [details]
Patch
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 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
Comment on attachment 333097 [details] Patch Clearing flags on attachment: 333097 Committed r228152: <https://trac.webkit.org/changeset/228152> All reviewed patches have been landed. Closing bug. |
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