This flaky assertion failure was seen with imported/w3c/web-platform-tests/service-workers/service-worker/getregistration.https.html ASSERTION FAILED: registration /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/workers/service/server/SWServerJobQueue.cpp(114) : void WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier) 1 0x11ebc87cd WTFCrash 2 0x112b8a76e WebCore::SWServerJobQueue::scriptContextStarted(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>) 3 0x112b8a705 WebCore::SWServer::scriptContextStarted(WebCore::SWServerWorker&) 4 0x112b8d7a4 WebCore::SWServerWorker::scriptContextStarted() 5 0x112b8d779 WebCore::SWServerToContextConnection::scriptContextStarted(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>) 6 0x10b877601 void IPC::callMemberFunctionImpl<WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType> >, 0ul>(WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType> >&&, std::__1::integer_sequence<unsigned long, 0ul>) 7 0x10b877558 void IPC::callMemberFunction<WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>), std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType> >, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType> >&&, WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>)) 8 0x10b876b61 void IPC::handleMessage<Messages::WebSWServerToContextConnection::ScriptContextStarted, WebKit::WebSWServerToContextConnection, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>)>(IPC::Decoder&, WebKit::WebSWServerToContextConnection*, void (WebCore::SWServerToContextConnection::*)(WTF::ObjectIdentifier<WebCore::ServiceWorkerIdentifierType>)) 9 0x10b87666d WebKit::WebSWServerToContextConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 10 0x10b1c499c WebKit::StorageProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 11 0x10ad35653 IPC::Connection::dispatchMessage(IPC::Decoder&) 12 0x10ad2c9f8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 13 0x10ad35c50 IPC::Connection::dispatchOneMessage() 14 0x10ad3cacd IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() 15 0x10ad3ca29 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() 16 0x11ec04bbb WTF::Function<void ()>::operator()() const 17 0x11ec25f33 WTF::RunLoop::performWork() 18 0x11ec26834 WTF::RunLoop::performWork(void*) 19 0x7fff8e1cc7e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 20 0x7fff8e1abf0c __CFRunLoopDoSources0 21 0x7fff8e1ab42f __CFRunLoopRun 22 0x7fff8e1aae28 CFRunLoopRunSpecific 23 0x7fff90a8fcb9 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] 24 0x7fff90a8fb98 -[NSRunLoop(NSRunLoop) run] 25 0x10ef96168 _xpc_objc_main 26 0x10ef94bbe xpc_main 27 0x10abf7045 main 28 0x7fff9835c5ad start 29 0x1 https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r224962%20(4200)/results.html
Skipped the test on debug in https://trac.webkit.org/r225004
Will look into this soon.
Created attachment 327329 [details] Patch
*** Bug 179748 has been marked as a duplicate of this bug. ***
Comment on attachment 327329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327329&action=review > Source/WTF/wtf/ObjectIdentifier.h:96 > + static uint64_t currentIdentifier = 0; No need for the "= 0" here. > Source/WTF/wtf/ObjectIdentifier.h:100 > +template<typename T> inline ObjectIdentifier<T> generateThreadSafeObjectIdentifier() Nothing prevents us from accidentally using generateThreadSafeObjectIdentifier and generateObjectIdentifier for the same type. Might be nice if there was something that made sure that never happened. > Source/WTF/wtf/ObjectIdentifier.h:102 > + static NeverDestroyed<std::atomic<uint64_t>> currentIdentifier; Does std::atomic have a non-trivial destructor? I am surprised we have to use NeverDestroyed here. What prevents two threads from racing to construct the std::atomic<uint64_t>? We turn off the code that normally protects globals from this kind of problem with a compiler flag. I fear that while access to the atomic is safely guarded against use from multiple threads, construction may not be. > Source/WebCore/workers/service/ServiceWorkerJobDataIdentifier.h:41 > + bool operator==(const ServiceWorkerJobDataIdentifier& other) const > + { > + return connectionIdentifier == other.connectionIdentifier && jobIdentifier == other.jobIdentifier; > + } Stylistically, I slightly prefer to make this a non-member function. Generally speaking it’s better because then conversions can happen on the left side too, and no special access to the struct internals is needed.
Comment on attachment 327329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327329&action=review >> Source/WTF/wtf/ObjectIdentifier.h:96 >> + static uint64_t currentIdentifier = 0; > > No need for the "= 0" here. I know. I kind of link it for clarity though but OK. >> Source/WTF/wtf/ObjectIdentifier.h:100 >> +template<typename T> inline ObjectIdentifier<T> generateThreadSafeObjectIdentifier() > > Nothing prevents us from accidentally using generateThreadSafeObjectIdentifier and generateObjectIdentifier for the same type. Might be nice if there was something that made sure that never happened. I had an initial patch with 2 different Identifier types (ObjectIdentifier & ThreadSafeObjectIdentifier) but it seemed overkill for this. >> Source/WTF/wtf/ObjectIdentifier.h:102 >> + static NeverDestroyed<std::atomic<uint64_t>> currentIdentifier; > > Does std::atomic have a non-trivial destructor? I am surprised we have to use NeverDestroyed here. > > What prevents two threads from racing to construct the std::atomic<uint64_t>? We turn off the code that normally protects globals from this kind of problem with a compiler flag. I fear that while access to the atomic is safely guarded against use from multiple threads, construction may not be. The compiler had me use NeverDestroyed here. I fear that you are indeed right about the thread safety issue. The thing is though that I am porting a class from using ThreadSafeIdentified to ObjectIdentifier<> with generateThreadSafeObjectIdentifier(). I followed the pattern used in ThreadSafeIdentified ( a global static with an std::atomic<uint64_t>. So the good news is that I did not make it worse. The bad news is that all our classes inheriting from ThreadSafeIdentified suffer from the same initialization issue. I will look into it but I don't know off-hand how to fix this. Note that I tried to maintain the thread-safety for the job identifier because Brady used ThreadSafeIdentified when introducing the code. However, we do not technically need this thread-safety yet. This is forward-looking for when we will support service workers inside workers. >> Source/WebCore/workers/service/ServiceWorkerJobDataIdentifier.h:41 >> + } > > Stylistically, I slightly prefer to make this a non-member function. Generally speaking it’s better because then conversions can happen on the left side too, and no special access to the struct internals is needed. Ok.
Comment on attachment 327329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327329&action=review >>> Source/WTF/wtf/ObjectIdentifier.h:102 >>> + static NeverDestroyed<std::atomic<uint64_t>> currentIdentifier; >> >> Does std::atomic have a non-trivial destructor? I am surprised we have to use NeverDestroyed here. >> >> What prevents two threads from racing to construct the std::atomic<uint64_t>? We turn off the code that normally protects globals from this kind of problem with a compiler flag. I fear that while access to the atomic is safely guarded against use from multiple threads, construction may not be. > > The compiler had me use NeverDestroyed here. > > I fear that you are indeed right about the thread safety issue. The thing is though that I am porting a class from using ThreadSafeIdentified to ObjectIdentifier<> with generateThreadSafeObjectIdentifier(). I followed the pattern used in ThreadSafeIdentified ( a global static with an std::atomic<uint64_t>. So the good news is that I did not make it worse. The bad news is that all our classes inheriting from ThreadSafeIdentified suffer from the same initialization issue. > > I will look into it but I don't know off-hand how to fix this. Note that I tried to maintain the thread-safety for the job identifier because Brady used ThreadSafeIdentified when introducing the code. However, we do not technically need this thread-safety yet. This is forward-looking for when we will support service workers inside workers. Looking at our code base, we seem to use std::call_once() for such case. e.g.: static FunctionWhitelist& ensureGlobalDFGWhitelist() { static LazyNeverDestroyed<FunctionWhitelist> dfgWhitelist; static std::once_flag initializeWhitelistFlag; std::call_once(initializeWhitelistFlag, [] { const char* functionWhitelistFile = Options::dfgWhitelist(); dfgWhitelist.construct(functionWhitelistFile); }); return dfgWhitelist; } or HashMap<const char*, GCTypeMap<SimpleStats>>& timingStats() { static HashMap<const char*, GCTypeMap<SimpleStats>>* result; static std::once_flag once; std::call_once( once, [] { result = new HashMap<const char*, GCTypeMap<SimpleStats>>(); }); return *result; }
Created attachment 327338 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 327338 [details]: http/tests/websocket/tests/hybi/workers/worker-reload.html bug 179874 (authors: abarth@webkit.org, rniwa@webkit.org, and yutak@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 327338 [details] Patch Clearing flags on attachment: 327338 Committed r225031: <https://trac.webkit.org/changeset/225031>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35635700>