Bug 197856 - Add assertions to help diagnose crash at WebProcessProxy::processPool()
Summary: Add assertions to help diagnose crash at WebProcessProxy::processPool()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-13 15:44 PDT by Sihui Liu
Modified: 2019-05-15 14:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.42 KB, patch)
2019-05-13 15:48 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2019-05-14 14:10 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.32 MB, application/zip)
2019-05-14 15:32 PDT, Build Bot
no flags Details
Patch for landing (4.27 KB, patch)
2019-05-15 12:10 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-05-13 15:44:12 PDT
-> 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.
Comment 1 Sihui Liu 2019-05-13 15:46:32 PDT
<rdar://problem/49341366>
Comment 2 Sihui Liu 2019-05-13 15:48:42 PDT
Created attachment 369795 [details]
Patch
Comment 3 Chris Dumez 2019-05-14 09:06:13 PDT
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.
Comment 4 Sihui Liu 2019-05-14 14:10:15 PDT
Created attachment 369890 [details]
Patch
Comment 5 Sihui Liu 2019-05-14 14:36:05 PDT
(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.
Comment 6 Chris Dumez 2019-05-14 14:55:45 PDT
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");
Comment 7 Build Bot 2019-05-14 15:32:47 PDT
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
Comment 8 Build Bot 2019-05-14 15:32:49 PDT
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
Comment 9 Sihui Liu 2019-05-15 12:10:15 PDT
Created attachment 369980 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-05-15 12:51:01 PDT
Comment on attachment 369980 [details]
Patch for landing

Clearing flags on attachment: 369980

Committed r245339: <https://trac.webkit.org/changeset/245339>
Comment 11 WebKit Commit Bot 2019-05-15 12:51:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Shawn Roberts 2019-05-15 14:09:40 PDT
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> >&&)
Comment 13 Chris Dumez 2019-05-15 14:12:00 PDT
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.
Comment 14 Chris Dumez 2019-05-15 14:15:02 PDT
Fixed assertion in <https://trac.webkit.org/changeset/245343>.