Bug 197856 – Add assertions to help diagnose crash at WebProcessProxy::processPool()

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

-> 119 	    WebProcessPool& processPool() const { ASSERT(m_processPool); return *m_processPool.get(); }

Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread
0   WebKit::WebsiteDataStore::processPools(unsigned long, bool) const + 114
1   WebKit::WebsiteDataStore::plugins() const + 53
2   WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue, WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::Function<void (WTF::Vector<WebKit::WebsiteDataRecord, 0ul, WTF::CrashOnOverflow, 16ul>)>&&) + 2024
3   WebKit::WebsiteDataStore::fetchDataForRegistrableDomains(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::Vector<WebCore::RegistrableDomain, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::CompletionHandler<void (WTF::Vector<WebKit::WebsiteDataRecord, 0ul, WTF::CrashOnOverflow, 16ul>&&, WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> >&&)>&&) + 167
4   WebKit::NetworkProcessProxy::deleteWebsiteDataInUIProcessForRegistrableDomains(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::Vector<WebCore::RegistrableDomain, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::CompletionHandler<void (WTF::HashSet<WebCore::RegistrableDomain, WebCore::RegistrableDomain::RegistrableDomainHash, WTF::HashTraits<WebCore::RegistrableDomain> >&&)>&&) + 130
5   WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) + 9056

It seems m_processPool is nullptr in the crash trace above.

[Sihui Liu]

<rdar://problem/49341366>

[Sihui Liu]

Created attachment 369795 [details]
Patch

[Chris Dumez]

Comment on attachment 369795 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessCache.cpp:249
> +    RELEASE_ASSERT(!m_process->pageCount());

Also:
RELEASE_ASSERT(!m_process->websiteDataStore().processes().contains(process.ptr());

> Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:46
> +    RELEASE_ASSERT(!process.isInProcessCache() && !process.isPrewarmed());

Please use 2 separate assertions so that we know which condition is true if it hits.

> Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:49
> +        RELEASE_LOG(Loading, "WebProcessLifetimeObserver::addWebPage: webPID = %i, pageID = %" PRIu64, process.processIdentifier(), webPageProxy.pageID());

I think you need to print |this| pointer too.

> Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:65
> +        RELEASE_LOG(Loading, "WebProcessLifetimeObserver::removeWebPage: webPID = %i, pageID = %" PRIu64, process.processIdentifier(), webPageProxy.pageID());

I think you need to print |this| pointer too.

[Sihui Liu]

Created attachment 369890 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 369795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369795&action=review
> 
> > Source/WebKit/UIProcess/WebProcessCache.cpp:249
> > +    RELEASE_ASSERT(!m_process->pageCount());
> 
> Also:
> RELEASE_ASSERT(!m_process->websiteDataStore().processes().contains(process.
> ptr());

Okay, processes() does not have contains. Adding a helper function.

>
> > Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:46
> > +    RELEASE_ASSERT(!process.isInProcessCache() && !process.isPrewarmed());
> 
> Please use 2 separate assertions so that we know which condition is true if
> it hits.

Sure.

>
> > Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:49
> > +        RELEASE_LOG(Loading, "WebProcessLifetimeObserver::addWebPage: webPID = %i, pageID = %" PRIu64, process.processIdentifier(), webPageProxy.pageID());
> 
> I think you need to print |this| pointer too.

Okay.

>
> > Source/WebKit/UIProcess/WebProcessLifetimeObserver.cpp:65
> > +        RELEASE_LOG(Loading, "WebProcessLifetimeObserver::removeWebPage: webPID = %i, pageID = %" PRIu64, process.processIdentifier(), webPageProxy.pageID());
> 
> I think you need to print |this| pointer too.

Okay.

[Chris Dumez]

Comment on attachment 369890 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessCache.cpp:250
> +    RELEASE_ASSERT(!m_process->websiteDataStore().hasProcess(process.ptr()));

RELEASE_ASSERT_WITH_MESSAGE(!m_process->websiteDataStore().hasProcess(process.ptr()), "Only processes with pages should be registered with the data store");

[EWS Watchlist]

Comment on attachment 369890 [details]
Patch

Attachment 369890 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12191661

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 369900 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Sihui Liu]

Created attachment 369980 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369980 [details]
Patch for landing

Clearing flags on attachment: 369980

Committed r245339: <https://trac.webkit.org/changeset/245339>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

After changes in https://trac.webkit.org/changeset/245339 , seeing 79 API test crashes on several queues. 

https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/4988/steps/run-api-tests/logs/stdio

run-api-tests TestWebKitAPI.ProcessSwap.NavigateBackAfterClientSideRedirect --debug

Hits the following Assertion with r245339, passes in prior builds.

ASSERTION FAILED: m_ptr
        /Volumes/Data/worker/liberty-debug-archive/build/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/Ref.h(120) : T *WTF::Ref<WebKit::WebProcessProxy, WTF::DumbPtrTraits<WebKit::WebProcessProxy> >::ptr() const [T = WebKit::WebProcessProxy, PtrTraits = WTF::DumbPtrTraits<WebKit::WebProcessProxy>]
        1   0x1133c4429 WTFCrash
        2   0x118e0ebcb WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x1196b53e8 WTF::Ref<WebKit::WebProcessProxy, WTF::DumbPtrTraits<WebKit::WebProcessProxy> >::ptr() const
        4   0x1199219f1 WebKit::WebProcessCache::CachedProcess::CachedProcess(WTF::Ref<WebKit::WebProcessProxy, WTF::DumbPtrTraits<WebKit::WebProcessProxy> >&&)

[Chris Dumez]

Comment on attachment 369980 [details]
Patch for landing

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

> Source/WebKit/UIProcess/WebProcessCache.cpp:250
> +    RELEASE_ASSERT_WITH_MESSAGE(!m_process->websiteDataStore().hasProcess(process.ptr()), "Only processes with pages should be registered with the data store");

use-after-move here. Should be m_process.ptr(), not process.ptr(). Sorry I missed that.

[Chris Dumez]

Fixed assertion in <https://trac.webkit.org/changeset/245343>.