Bug 200986 – Crash under StringImpl::~StringImpl() in NetworkProcess::deleteWebsiteDataForRegistrableDomains()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Crash under StringImpl::~StringImpl() in NetworkProcess::deleteWebsiteDataForRegistrableDomains():
Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x656e6e6f43726576 -> 0x0000006f43726576 (possible pointer authentication failure))
ok
[  0] 0x00000001adf143f4 JavaScriptCore`WTF::StringImpl::~StringImpl() [inlined] WTF::Function<void (WTF::ExternalStringImpl*, void*, unsigned int)>::operator()(WTF::ExternalStringImpl*, void*, unsigned int) const + 4 at Function.h:79:35

     0x00000001adf143e4:      and x8, x8, #0x1
     0x00000001adf143e8:      lsl w8, w9, w8
     0x00000001adf143ec:      add w3, w8, #0x18        ; =0x18 
     0x00000001adf143f0:      ldr x0, [x19, #0x18]
 ->  0x00000001adf143f4:      ldr x8, [x0]
     0x00000001adf143f8:    ldraa x9, [x8, #0x10]!
     0x00000001adf143fc:     movk x8, #0x6e56, lsl #48
     0x00000001adf14400:      mov x1, x19
     0x00000001adf14404:    blraa x9, x8

[  0] 0x00000001adf143f0 JavaScriptCore`WTF::StringImpl::~StringImpl() [inlined] WTF::ExternalStringImpl::freeExternalBuffer(void*, unsigned int) at ExternalStringImpl.h:50
[  0] 0x00000001adf143f0 JavaScriptCore`WTF::StringImpl::~StringImpl() + 168 at StringImpl.cpp:138
       134 	        return;
       135 	    }
       136 	    if (ownership == BufferExternal) {
       137 	        auto* external = static_cast<ExternalStringImpl*>(this);
    -> 138 	        external->freeExternalBuffer(const_cast<LChar*>(m_data8), sizeInBytes());
       139 	        external->m_free.~ExternalStringImplFreeFunction();
       140 	        return;
       141 	    }
       142 	
    
[  1] 0x00000001adf1449f JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) [inlined] WTF::StringImpl::~StringImpl() + 3 at StringImpl.cpp:108:1
       104 	
       105 	StringImpl::StaticStringImpl StringImpl::s_emptyAtomString("", StringImpl::StringAtom);
       106 	
       107 	StringImpl::~StringImpl()
    -> 108 	{
       109 	    ASSERT(!isStatic());
       110 	
       111 	    StringView::invalidate(*this);
       112 	
    
[  1] 0x00000001adf1449c JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) + 12 at StringImpl.cpp:150
       146 	}
       147 	
       148 	void StringImpl::destroy(StringImpl* stringImpl)
       149 	{
    -> 150 	    stringImpl->~StringImpl();
       151 	    fastFree(stringImpl);
       152 	}
       153 	
       154 	Ref<StringImpl> StringImpl::createFromLiteral(const char* characters, unsigned length)
    
[  2] 0x00000001adf1449f JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) [inlined] WTF::StringImpl::~StringImpl() + 3 at StringImpl.cpp:108:1
       104 	
       105 	StringImpl::StaticStringImpl StringImpl::s_emptyAtomString("", StringImpl::StringAtom);
       106 	
       107 	StringImpl::~StringImpl()
    -> 108 	{
       109 	    ASSERT(!isStatic());
       110 	
       111 	    StringView::invalidate(*this);
       112 	
    
[  2] 0x00000001adf1449c JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) + 12 at StringImpl.cpp:150
       146 	}
       147 	
       148 	void StringImpl::destroy(StringImpl* stringImpl)
       149 	{
    -> 150 	    stringImpl->~StringImpl();
       151 	    fastFree(stringImpl);
       152 	}
       153 	
       154 	Ref<StringImpl> StringImpl::createFromLiteral(const char* characters, unsigned length)
    
ok
[  3] 0x00000001a676a1d7 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] WTF::StringImpl::deref() + 3 at StringImpl.h:1076:9
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] void WTF::derefIfNotNull<WTF::StringImpl>(WTF::StringImpl*) at RefPtr.h:44
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::~RefPtr() at RefPtr.h:69
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::~RefPtr() at RefPtr.h:69
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::operator=(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&) at RefPtr.h:165
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) [inlined] WTF::String::operator=(WTF::String&&) at WTFString.h:134
[  3] 0x00000001a676a1d4 WebKit`WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::HashMap<WebCore::RegistrableDomain, WebKit::WebsiteDataToRemove, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain>, WTF::HashTraits<WebKit::WebsiteDataToRemove> >&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> > const&)>&&) + 2356 at NetworkProcess.cpp:1739
       1735	    }
       1736	#endif
       1737	    
       1738	#if ENABLE(SERVICE_WORKER)
    -> 1739	    path = m_swDatabasePaths.get(sessionID);
       1740	    if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::ServiceWorkerRegistrations)) {
       1741	        swServerForSession(sessionID).getOriginsWithRegistrations([this, sessionID, domainsToDeleteAllButCookiesFor, callbackAggregator = callbackAggregator.copyRef()](const HashSet<SecurityOriginData>& securityOrigins) mutable {
       1742	            for (auto& securityOrigin : securityOrigins) {
       1743	                if (!domainsToDeleteAllButCookiesFor.contains(RegistrableDomain::uncheckedCreateFromHost(securityOrigin.host)))
    
[  4] 0x00000001a67b8de7 WebKit`WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_5, void>::call() [inlined] WebKit::ResourceLoadStatisticsStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_5::operator()() + 115 at ResourceLoadStatisticsStore.cpp:209:16
       205 	
       206 	    setDataRecordsBeingRemoved(true);
       207 	
       208 	    RunLoop::main().dispatch([store = makeRef(m_store), domainsToRemoveWebsiteDataFor = crossThreadCopy(domainsToRemoveWebsiteDataFor), completionHandler = WTFMove(completionHandler), weakThis = makeWeakPtr(*this), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
    -> 209 	        store->deleteWebsiteDataForRegistrableDomains(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(domainsToRemoveWebsiteDataFor), shouldNotifyPagesWhenDataRecordsWereScanned, [completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<RegistrableDomain>& domainsWithDeletedWebsiteData) mutable {
       210 	            workQueue->dispatch([domainsWithDeletedWebsiteData = crossThreadCopy(domainsWithDeletedWebsiteData), completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis)] () mutable {
       211 	                if (!weakThis) {
       212 	                    completionHandler();
       213 	                    return;
    
[  4] 0x00000001a67b8d74 WebKit`WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_5, void>::call() + 32 at Function.h:52
[  5] 0x00000001adf0fdd3 JavaScriptCore`WTF::RunLoop::performWork() [inlined] WTF::Function<void ()>::operator()() const + 19 at Function.h:79:35
[  5] 0x00000001adf0fdc0 JavaScriptCore`WTF::RunLoop::performWork() + 252 at RunLoop.cpp:106
       102 	
       103 	            function = m_functionQueue.takeFirst();
       104 	        }
       105 	
    -> 106 	        function();
       107 	    }
       108 	
       109 	    for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; ++functionsHandled) {
       110 	        Function<void ()> function;
    
[  6] 0x00000001adf100a3 JavaScriptCore`WTF::RunLoop::performWork(void*) + 39 at RunLoopCF.cpp:38:37
       34  	
       35  	void RunLoop::performWork(void* context)
       36  	{
       37  	    AutodrainedPool pool;
    -> 38  	    static_cast<RunLoop*>(context)->performWork();
       39  	}
       40  	
       41  	RunLoop::RunLoop()
       42  	    : m_runLoop(CFRunLoopGetCurrent())
    
ok
[  7] 0x000000019f058b3f CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 27 at CFRunLoop.c:1922:9
       1918	
       1919	static void __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__(void (*)(void *), void *) __attribute__((noinline));
       1920	static void __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__(void (*perform)(void *), void *info) {
       1921	    if (perform) {
    -> 1922	        perform(info);
       1923	    }
       1924	    __asm __volatile__(""); // thwart tail-call optimization
       1925	}
       1926

[Chris Dumez]

<rdar://problem/32850192>

[Chris Dumez]

Created attachment 376894 [details]
Patch

[Brent Fulgham]

Comment on attachment 376894 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        does not take care of this for you, despite its name (the createCrossThreadTask() function does though).

Ugh!

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1364
>              RunLoop::main().dispatch([callbackAggregator = WTFMove(callbackAggregator), securityOrigins = indexedDatabaseOrigins(path)] {

This makes sense to me; path is created on the main thread, and needs to be copied for use in the cross-thread task.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1734
> +            RunLoop::main().dispatch([this, sessionID, domainsToDeleteAllButCookiesFor = WTFMove(domainsToDeleteAllButCookiesFor), callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {

So it looks like we had this thread-transfer backwards in the original code. I wonder how we can make it harder to avoid this problem.

I understand why we need to crossThreadCopy path (created on the main thread) to the cross-thread task (line 1733), but why is it okay to WTFMove that cross-thread copied memory back to the main thread on line 1734?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1878
> +        postStorageTask(CrossThreadTask([this, callbackAggregator = callbackAggregator.copyRef(), path = crossThreadCopy(path)]() mutable {

Again, this one makes sense.

[Chris Dumez]

Comment on attachment 376894 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1734
>> +            RunLoop::main().dispatch([this, sessionID, domainsToDeleteAllButCookiesFor = WTFMove(domainsToDeleteAllButCookiesFor), callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {
> 
> So it looks like we had this thread-transfer backwards in the original code. I wonder how we can make it harder to avoid this problem.
> 
> I understand why we need to crossThreadCopy path (created on the main thread) to the cross-thread task (line 1733), but why is it okay to WTFMove that cross-thread copied memory back to the main thread on line 1734?

Technically, we could crossThreadCopy() both, however, I know WTFMove() is safe here because I have just isolatedCopied the data structure already and I did not pass it to anybody else.
It is safe to WTFMove() a String to another thread if:
1. RefCount is 1 (i.e. String is not shared with anybody else)
and
2. String is not atomized

See String::isSafeToSendToAnotherThread().

[Brent Fulgham]

Comment on attachment 376894 [details]
Patch

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

r=me

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1734
>>> +            RunLoop::main().dispatch([this, sessionID, domainsToDeleteAllButCookiesFor = WTFMove(domainsToDeleteAllButCookiesFor), callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {
>> 
>> So it looks like we had this thread-transfer backwards in the original code. I wonder how we can make it harder to avoid this problem.
>> 
>> I understand why we need to crossThreadCopy path (created on the main thread) to the cross-thread task (line 1733), but why is it okay to WTFMove that cross-thread copied memory back to the main thread on line 1734?
> 
> Technically, we could crossThreadCopy() both, however, I know WTFMove() is safe here because I have just isolatedCopied the data structure already and I did not pass it to anybody else.
> It is safe to WTFMove() a String to another thread if:
> 1. RefCount is 1 (i.e. String is not shared with anybody else)
> and
> 2. String is not atomized
> 
> See String::isSafeToSendToAnotherThread().

Sounds good.

[WebKit Commit Bot]

Comment on attachment 376894 [details]
Patch

Clearing flags on attachment: 376894

Committed r248959: <https://trac.webkit.org/changeset/248959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.