Bug 181603 – ASSERTION FAILED: registration || isTerminating() in WebCore::SWServerWorker::skipWaiting()

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

Created attachment 331217 [details]
Crash log

This crash is seen with LayoutTest http/tests/workers/service/postmessage-after-sw-process-crash.https.html

ASSERTION FAILED: registration || isTerminating()
/Volumes/Data/slave/highsierra-debug/build/Source/WebCore/workers/service/server/SWServerWorker.cpp(136) : void WebCore::SWServerWorker::skipWaiting()
1   0x11c40e50d WTFCrash
2   0x110809105 WebCore::SWServerWorker::skipWaiting()
3   0x110809075 WebCore::SWServerToContextConnection::skipWaiting(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long)
4   0x107355a84 void IPC::callMemberFunctionImpl<WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long>, 0ul, 1ul>(WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
5   0x107355820 void IPC::callMemberFunction<WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long>&&, WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long))
6   0x10735408f void IPC::handleMessage<Messages::WebSWServerToContextConnection::SkipWaiting, WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long)>(IPC::Decoder&, WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>, unsigned long long))
7   0x107353652 WebKit::WebSWServerToContextConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
8   0x106beca44 WebKit::StorageToWebProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
9   0x106667c53 IPC::Connection::dispatchMessage(IPC::Decoder&)
10  0x10665d2d8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
11  0x10666825a IPC::Connection::dispatchOneMessage()
12  0x10668071d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
13  0x106680679 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
14  0x11c42a65b WTF::Function<void ()>::operator()() const
15  0x11c46f543 WTF::RunLoop::performWork()
16  0x11c46fde4 WTF::RunLoop::performWork(void*)
17  0x7fff564c2711 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
18  0x7fff5657a39c __CFRunLoopDoSource0
19  0x7fff564a5700 __CFRunLoopDoSources0
20  0x7fff564a4b7d __CFRunLoopRun
21  0x7fff564a43d7 CFRunLoopRunSpecific
22  0x7fff5859eb76 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
23  0x7fff5859ea4e -[NSRunLoop(NSRunLoop) run]
24  0x7fff7e116403 _xpc_objc_main
25  0x7fff7e115082 xpc_main
26  0x10651e12b main
27  0x7fff7de49115 start

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r226886%20(1585)/results.html

[Ryan Haddad]

This assert was added in https://trac.webkit.org/changeset/226813

[Radar WebKit Bug Importer]

<rdar://problem/36476050>

[Chris Dumez]

Darn.

[Chris Dumez]

I managed to reproduce and the state is "Running", which is unexpected.

[Chris Dumez]

Created attachment 331241 [details]
Patch

[youenn fablet]

Comment on attachment 331241 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:567
> +        workerContextTerminated(worker);

Is this line needed?
Should we add an ASSERT(connection) as well before if (!connection)?

I would believe that if the connection is null, there probably was a connection before and it was destroyed (broken IPC for instance).
In such a case, I would have expected that its destructor would clean/terminate all its related workers.

[Chris Dumez]

(In reply to youenn fablet from comment #6)
> Comment on attachment 331241 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331241&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:567
> > +        workerContextTerminated(worker);
> 
> Is this line needed?
> Should we add an ASSERT(connection) as well before if (!connection)?
> 
> I would believe that if the connection is null, there probably was a
> connection before and it was destroyed (broken IPC for instance).
> In such a case, I would have expected that its destructor would
> clean/terminate all its related workers.

Yes, it is needed. I will investigate why in a follow up. For now, I'll land this to make the bots happy.

[WebKit Commit Bot]

Comment on attachment 331241 [details]
Patch

Clearing flags on attachment: 331241

Committed r226934: <https://trac.webkit.org/changeset/226934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to youenn fablet from comment #6)
> > Comment on attachment 331241 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331241&action=review
> > 
> > > Source/WebCore/workers/service/server/SWServer.cpp:567
> > > +        workerContextTerminated(worker);
> > 
> > Is this line needed?
> > Should we add an ASSERT(connection) as well before if (!connection)?
> > 
> > I would believe that if the connection is null, there probably was a
> > connection before and it was destroyed (broken IPC for instance).
> > In such a case, I would have expected that its destructor would
> > clean/terminate all its related workers.
> 
> Yes, it is needed. I will investigate why in a follow up. For now, I'll land
> this to make the bots happy.

Found the reason, follow-up is at Bug 181687.

[Ryan Haddad]

Still seeing this assertion failure on the bots:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r226977%20(1628)/results.html