Summary: | ASSERTION FAILED: registration in WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||
Component: | New Bugs | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | beidson, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, ggaren, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=179877 | ||||||||
Attachments: |
|
Description
Ryan Haddad
2017-11-17 16:03:34 PST
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. |