Bug 179846 - ASSERTION FAILED: registration in WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier)
Summary: ASSERTION FAILED: registration in WebCore::SWServerJobQueue::scriptContextSta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 179748 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-11-17 16:03 PST by Ryan Haddad
Modified: 2017-11-19 14:48 PST (History)
11 users (show)

See Also:


Attachments
Patch (77.30 KB, patch)
2017-11-18 17:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (77.31 KB, patch)
2017-11-18 21:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2017-11-17 16:03:34 PST
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
Comment 1 Ryan Haddad 2017-11-17 17:08:53 PST
Skipped the test on debug in https://trac.webkit.org/r225004
Comment 2 Chris Dumez 2017-11-17 18:20:01 PST
Will look into this soon.
Comment 3 Chris Dumez 2017-11-18 17:59:47 PST
Created attachment 327329 [details]
Patch
Comment 4 Chris Dumez 2017-11-18 18:00:41 PST
*** Bug 179748 has been marked as a duplicate of this bug. ***
Comment 5 Darin Adler 2017-11-18 20:20:47 PST
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 6 Chris Dumez 2017-11-18 21:36:34 PST
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 7 Chris Dumez 2017-11-18 21:51:22 PST
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;
}
Comment 8 Chris Dumez 2017-11-18 21:59:27 PST
Created attachment 327338 [details]
Patch
Comment 9 WebKit Commit Bot 2017-11-18 22:46:57 PST
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 10 Chris Dumez 2017-11-18 23:19:05 PST
Comment on attachment 327338 [details]
Patch

Clearing flags on attachment: 327338

Committed r225031: <https://trac.webkit.org/changeset/225031>
Comment 11 Chris Dumez 2017-11-18 23:19:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-11-18 23:21:26 PST
<rdar://problem/35635700>