Bug 179846 – ASSERTION FAILED: registration in WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier)

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Ryan Haddad]

Skipped the test on debug in https://trac.webkit.org/r225004

[Chris Dumez]

Will look into this soon.

[Chris Dumez]

Created attachment 327329 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

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;
}

[Chris Dumez]

Created attachment 327338 [details]
Patch

[WebKit Commit Bot]

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: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Chris Dumez]

Comment on attachment 327338 [details]
Patch

Clearing flags on attachment: 327338

Committed r225031: <https://trac.webkit.org/changeset/225031>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35635700>