Bug 190060 – [iOS][WK1] ASSERTION FAILED: m_creationThread.ptr() == &Thread::current() in ServiceWorkerContainer::~ServiceWorkerContainer()

• Component: WebCore Misc.
• Status: NEW

[Daniel Bates]

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 ()

[youenn fablet]

The assert probably just needs to be updated to take into account main thread/web thread handling.

[Chris Dumez]

This is WK1, it should not have service workers enabled so we should not be constructing a ServiceWorkerContainer at all I believe.

[Chris Dumez]

Created attachment 351020 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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).

[Chris Dumez]

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.

[youenn fablet]

> 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?

[Chris Dumez]

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.

[Chris Dumez]

<rdar://problem/47038721>

[youenn fablet]

Created attachment 366902 [details]
Patch

[youenn fablet]

Created attachment 366937 [details]
Patch

[youenn fablet]

Created attachment 366942 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[Chris Dumez]

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.

[Chris Dumez]

(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.

[youenn fablet]

(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.