Bug 190060

Summary: [iOS][WK1] ASSERTION FAILED: m_creationThread.ptr() == &Thread::current() in ServiceWorkerContainer::~ServiceWorkerContainer()
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: NEW ---    
Severity: Normal CC: ap, benjamin, cdumez, cmarcelo, esprehn+autocc, ews, kangil.han, kondapallykalyan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 12   
See Also: https://bugs.webkit.org/show_bug.cgi?id=183682
https://bugs.webkit.org/show_bug.cgi?id=196692
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch youennf: review?

Description Daniel Bates 2018-09-27 15:46:32 PDT
Seen in the iPad Simulator running iOS 12 with a debug build WebKit at r236471. I was using an internal app that had a UIWebView and was about to enter a new URL when the app crashed in the WebThread due the assert m_creationThread.ptr() == &Thread::current() failing in ServiceWorkerContainer::~ServiceWorkerContainer():

WebThread (9)#0 0x000000010af33dd0 in ::WTFCrash() at /Volumes/.../OpenSource/Source/WTF/wtf/Assertions.cpp:255
#1  0x000000010f0b4adb in WTFCrashWithInfo(int, char const*, char const*, int) at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/Assertions.h:551
#2  0x00000001128aefef in WebCore::ServiceWorkerContainer::~ServiceWorkerContainer() at /Volumes/.../OpenSource/Source/WebCore/workers/service/ServiceWorkerContainer.cpp:70
#3  0x00000001128af1d5 in WebCore::ServiceWorkerContainer::~ServiceWorkerContainer() at /Volumes/.../OpenSource/Source/WebCore/workers/service/ServiceWorkerContainer.cpp:68
#4  0x00000001128af239 in WebCore::ServiceWorkerContainer::~ServiceWorkerContainer() at /Volumes/.../OpenSource/Source/WebCore/workers/service/ServiceWorkerContainer.cpp:68
#5  0x0000000111a390bf in std::__1::default_delete<WebCore::ServiceWorkerContainer>::operator()(WebCore::ServiceWorkerContainer*) const [inlined] at /Volumes/Xcode/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/include/c++/v1/memory:2285
#6  0x0000000111a390a0 in std::__1::unique_ptr<WebCore::ServiceWorkerContainer, std::__1::default_delete<WebCore::ServiceWorkerContainer> >::reset(WebCore::ServiceWorkerContainer*) [inlined] at /Volumes/Xcode/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/include/c++/v1/memory:2598
#7  0x0000000111a39053 in std::__1::unique_ptr<WebCore::ServiceWorkerContainer, std::__1::default_delete<WebCore::ServiceWorkerContainer> >::~unique_ptr() [inlined] at /Volumes/Xcode/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/include/c++/v1/memory:2552
#8  0x0000000111a39053 in std::__1::unique_ptr<WebCore::ServiceWorkerContainer, std::__1::default_delete<WebCore::ServiceWorkerContainer> >::~unique_ptr() [inlined] at /Volumes/Xcode/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/include/c++/v1/memory:2552
#9  0x0000000111a39053 in WTF::UniqueRef<WebCore::ServiceWorkerContainer>::~UniqueRef() at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/UniqueRef.h:42
#10 0x0000000111a34525 in WTF::UniqueRef<WebCore::ServiceWorkerContainer>::~UniqueRef() at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/UniqueRef.h:42
#11 0x0000000111a33511 in WebCore::NavigatorBase::~NavigatorBase() at /Volumes/.../OpenSource/Source/WebCore/page/NavigatorBase.cpp:88
#12 0x0000000111a33458 in WebCore::Navigator::~Navigator() at /Volumes/.../OpenSource/Source/WebCore/page/Navigator.cpp:58
#13 0x0000000111a33545 in WebCore::Navigator::~Navigator() at /Volumes/.../OpenSource/Source/WebCore/page/Navigator.cpp:58
#14 0x0000000111a33589 in WebCore::Navigator::~Navigator() at /Volumes/.../OpenSource/Source/WebCore/page/Navigator.cpp:58
#15 0x000000010fe1049f in WTF::RefCounted<WebCore::NavigatorBase>::deref() const at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/RefCounted.h:145
#16 0x000000010fe10423 in WTF::Ref<WebCore::Navigator, WTF::DumbPtrTraits<WebCore::Navigator> >::~Ref() at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/Ref.h:61
#17 0x000000010fe103e5 in WTF::Ref<WebCore::Navigator, WTF::DumbPtrTraits<WebCore::Navigator> >::~Ref() at /.../WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/Ref.h:55
#18 0x000000010fe103c9 in WebCore::JSDOMWrapper<WebCore::Navigator>::~JSDOMWrapper() at /Volumes/.../OpenSource/Source/WebCore/bindings/js/JSDOMWrapper.h:72
#19 0x000000010fe103a5 in WebCore::JSNavigator::~JSNavigator() at /.../WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/JSNavigator.h:29
#20 0x000000010fe0b395 in WebCore::JSNavigator::~JSNavigator() at /.../WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/JSNavigator.h:29
#21 0x000000010fe0a89d in WebCore::JSNavigator::destroy(JSC::JSCell*) at /.../WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/JSNavigator.cpp:379
#22 0x000000010bee558a in JSC::JSDestructibleObjectDestroyFunc::operator()(JSC::VM&, JSC::JSCell*) const at /Volumes/.../OpenSource/Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:37
#23 0x000000010bf204a5 in void JSC::MarkedBlock::Handle::specializedSweep<false, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)0, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::JSDestructibleObjectDestroyFunc>(JSC::FreeList*, JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::JSDestructibleObjectDestroyFunc const&)::'lambda'(void*)::operator()(void*) const at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/MarkedBlockInlines.h:260
#24 0x000000010bf20514 in void JSC::MarkedBlock::Handle::specializedSweep<false, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)0, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::JSDestructibleObjectDestroyFunc>(JSC::FreeList*, JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::JSDestructibleObjectDestroyFunc const&)::'lambda'(unsigned long)::operator()(unsigned long) const at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/MarkedBlockInlines.h:319
#25 0x000000010bf1b02a in void JSC::MarkedBlock::Handle::specializedSweep<false, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)0, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::JSDestructibleObjectDestroyFunc>(JSC::FreeList*, JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::JSDestructibleObjectDestroyFunc const&) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/MarkedBlockInlines.h:341
#26 0x000000010bee5520 in void JSC::MarkedBlock::Handle::finishSweepKnowingHeapCellType<JSC::JSDestructibleObjectDestroyFunc>(JSC::FreeList*, JSC::JSDestructibleObjectDestroyFunc const&) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/MarkedBlockInlines.h:439
#27 0x000000010bee53e8 in JSC::JSDestructibleObjectHeapCellType::finishSweep(JSC::MarkedBlock::Handle&, JSC::FreeList*) at /Volumes/.../OpenSource/Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:52
#28 0x000000010ba55656 in JSC::Subspace::finishSweep(JSC::MarkedBlock::Handle&, JSC::FreeList*) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/Subspace.cpp:65
#29 0x000000010ba38aab in JSC::MarkedBlock::Handle::sweep(JSC::FreeList*) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/MarkedBlock.cpp:432
#30 0x000000010ba0b4b8 in JSC::IncrementalSweeper::sweepNextBlock(JSC::VM&) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/IncrementalSweeper.cpp:89
#31 0x000000010ba0b36a in JSC::IncrementalSweeper::doSweep(JSC::VM&, WTF::MonotonicTime) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/IncrementalSweeper.cpp:59
#32 0x000000010ba0b32c in JSC::IncrementalSweeper::doWork(JSC::VM&) at /Volumes/.../OpenSource/Source/JavaScriptCore/heap/IncrementalSweeper.cpp:54
#33 0x000000010bf7c621 in JSC::JSRunLoopTimer::timerDidFire() at /Volumes/.../OpenSource/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:307
#34 0x000000010bf7bb47 in JSC::JSRunLoopTimer::Manager::timerDidFire() at /Volumes/.../OpenSource/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:160
#35 0x000000010bf7b5ec in JSC::JSRunLoopTimer::Manager::timerDidFireCallback(__CFRunLoopTimer*, void*) at /Volumes/.../OpenSource/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:61
#36 0x00000001064ad344 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ ()
#37 0x00000001064acf42 in __CFRunLoopDoTimer ()
#38 0x00000001064ac7aa in __CFRunLoopDoTimers ()
#39 0x00000001064a6e2c in __CFRunLoopRun ()
#40 0x00000001064a6221 in CFRunLoopRunSpecific ()
#41 0x000000010fa2b98a in RunWebThread(void*) at /Volumes/.../OpenSource/Source/WebCore/platform/ios/wak/WebCoreThread.mm:612
#42 0x000000011e9ad33d in _pthread_body ()
#43 0x000000011e9b02a7 in _pthread_start ()
#44 0x000000011e9ac425 in thread_start ()
Comment 1 youenn fablet 2018-09-27 16:32:48 PDT
The assert probably just needs to be updated to take into account main thread/web thread handling.
Comment 2 Chris Dumez 2018-09-27 16:37:04 PDT
This is WK1, it should not have service workers enabled so we should not be constructing a ServiceWorkerContainer at all I believe.
Comment 3 Chris Dumez 2018-09-27 16:57:15 PDT
Created attachment 351020 [details]
Patch
Comment 4 youenn fablet 2018-09-27 18:18:06 PDT
Comment on attachment 351020 [details]
Patch

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

> Source/WebCore/page/NavigatorBase.cpp:155
> +    return m_serviceWorkerContainer.get();

I am not sure this is worth the effort.
If we go with it, I would keep returning a ServiceWorkerContainer&.
We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method.
Comment 5 Chris Dumez 2018-09-28 09:03:33 PDT
Comment on attachment 351020 [details]
Patch

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

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:34
> +    visitor.addOpaqueRoot(wrapped().serviceWorker());

Note that we currently do not need a RuntimeEnabledCheck here...

> Source/WebCore/bindings/js/JSWorkerNavigatorCustom.cpp:34
> +    visitor.addOpaqueRoot(wrapped().serviceWorker());

... or here.

> Source/WebCore/dom/ScriptExecutionContext.cpp:569
> +    return navigator ? navigator->serviceWorker() : nullptr;

or here.

>> Source/WebCore/page/NavigatorBase.cpp:155
>> +    return m_serviceWorkerContainer.get();
> 
> I am not sure this is worth the effort.
> If we go with it, I would keep returning a ServiceWorkerContainer&.
> We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method.

If we keep returning a ServiceWorkerContainer& here then we'd have to check RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call sites (e.g. in GC visitors).
Comment 6 Chris Dumez 2018-09-28 09:10:10 PDT
Comment on attachment 351020 [details]
Patch

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

>>> Source/WebCore/page/NavigatorBase.cpp:155
>>> +    return m_serviceWorkerContainer.get();
>> 
>> I am not sure this is worth the effort.
>> If we go with it, I would keep returning a ServiceWorkerContainer&.
>> We could assert RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() and create m_serviceWorkerContainer in this method.
> 
> If we keep returning a ServiceWorkerContainer& here then we'd have to check RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call sites (e.g. in GC visitors).

Also, if we initialized m_serviceWorkerContainer in this method, then we'd need an extra member to store the navigator's scriptExecutionContext since the ServiceWorkerContainer constructor requires it.
Comment 7 youenn fablet 2018-09-28 14:27:21 PDT
> If we keep returning a ServiceWorkerContainer& here then we'd have to check
> RuntimeEnabledFeatures::sharedFeatures().serviceWorkerEnabled() at call
> sites (e.g. in GC visitors).

Maybe a "ServiceWorkerContainer* serviceWorkerContainerIfAny()" would do the trick for these?

As of the ScriptExecutionContext, can it be passed to the may-create getter?
Comment 8 Chris Dumez 2018-09-28 14:31:08 PDT
Comment on attachment 351020 [details]
Patch

I do not really understand the problem. Returning a raw pointer adds basically no complexity at call sites, as you can see in my patch.
Comment 9 Chris Dumez 2019-01-04 15:40:03 PST
<rdar://problem/47038721>
Comment 10 youenn fablet 2019-04-06 21:47:31 PDT
Created attachment 366902 [details]
Patch
Comment 11 youenn fablet 2019-04-08 08:45:43 PDT
Created attachment 366937 [details]
Patch
Comment 12 youenn fablet 2019-04-08 09:43:36 PDT
Created attachment 366942 [details]
Patch
Comment 13 Chris Dumez 2019-04-08 13:14:59 PDT
Comment on attachment 366942 [details]
Patch

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

> Source/WTF/wtf/ThreadObserver.h:33
> +class ThreadObserver {

The name is very generic considering what it does. I am also not convinced we need a new class just for this? I am even more confused this is used as a data member and not a base class.
Comment 14 youenn fablet 2019-04-09 13:13:18 PDT
(In reply to Chris Dumez from comment #13)
> Comment on attachment 366942 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366942&action=review
> 
> > Source/WTF/wtf/ThreadObserver.h:33
> > +class ThreadObserver {
> 
> The name is very generic considering what it does. I am also not convinced
> we need a new class just for this? I am even more confused this is used as a
> data member and not a base class.

I thought of ThreadChecker initially and can change to that.

This can be used as a base class although I do not see how much it improves things there.

As of its use, we could try to use something like that for all our WKWebView delegates taking completion handler.
We could check that they are called and destroyed on the thread they are created on.
Comment 15 Chris Dumez 2019-04-09 13:24:17 PDT
Comment on attachment 366942 [details]
Patch

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

>>> Source/WTF/wtf/ThreadObserver.h:33
>>> +class ThreadObserver {
>> 
>> The name is very generic considering what it does. I am also not convinced we need a new class just for this? I am even more confused this is used as a data member and not a base class.
> 
> I thought of ThreadChecker initially and can change to that.
> 
> This can be used as a base class although I do not see how much it improves things there.
> 
> As of its use, we could try to use something like that for all our WKWebView delegates taking completion handler.
> We could check that they are called and destroyed on the thread they are created on.

This class does not check anything though. All it does is remember the creation thread. You could have "checked" in a destructor that the object gets destroyed on the creation thread but you did not.

Also note that ServiceWorkerContainer is already an ActiveDOMObject and that ActiveDOMObject already saves m_creationThread.
Comment 16 Chris Dumez 2019-04-09 13:28:29 PDT
(In reply to youenn fablet from comment #14)
> (In reply to Chris Dumez from comment #13)
> > Comment on attachment 366942 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=366942&action=review
> > 
> > > Source/WTF/wtf/ThreadObserver.h:33
> > > +class ThreadObserver {
> > 
> > The name is very generic considering what it does. I am also not convinced
> > we need a new class just for this? I am even more confused this is used as a
> > data member and not a base class.
> 
> I thought of ThreadChecker initially and can change to that.

> This can be used as a base class although I do not see how much it improves
> things there.

It would improve things before:
1. You would not need a data member in the subclass
2. You could just call isCreationThread() instead of m_dataMember.isCreationThread()
3. You could add a destructor to ThreadObserver (or whatever it should be named) to make sure it gets destroyed on the creation thread, so that subclasses do not have to.

In any case, my preference would be to not add a class in WTF for this at all.
Comment 17 youenn fablet 2019-04-29 16:53:21 PDT
(In reply to Chris Dumez from comment #16)
> (In reply to youenn fablet from comment #14)
> > (In reply to Chris Dumez from comment #13)
> > > Comment on attachment 366942 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=366942&action=review
> > > 
> > > > Source/WTF/wtf/ThreadObserver.h:33
> > > > +class ThreadObserver {
> > > 
> > > The name is very generic considering what it does. I am also not convinced
> > > we need a new class just for this? I am even more confused this is used as a
> > > data member and not a base class.
> > 
> > I thought of ThreadChecker initially and can change to that.
> 
> > This can be used as a base class although I do not see how much it improves
> > things there.
> 
> It would improve things before:
> 1. You would not need a data member in the subclass

A data member in the subclass is not needed in either cases.
Availability to subclass in both cases can be public, private or protected.

> 3. You could add a destructor to ThreadObserver (or whatever it should be
> named) to make sure it gets destroyed on the creation thread, so that
> subclasses do not have to.

Sure, I had this in mind, for instance to issues we faced with callback destructors.
I do not see how it makes a difference regarding the subclass or data member issue though.

> In any case, my preference would be to not add a class in WTF for this at
> all.

I do not feel strongly about this class.
When looking at the code before and after the patch, it seems cleaner after without all these #ifdef NDEBUG.
If we start doing more of these checks in the future, it seems like a nice addition to me.