Bug 198935

Summary: Crash at com.apple.WebKit: WebKit::WebsiteDataStore::processPools const
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, ggaren
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Sihui Liu
Reported 2019-06-17 15:21:09 PDT
Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x00007fff4cffeda0 WebKit::WebsiteDataStore::processPools(unsigned long, bool) const + 114 1 com.apple.WebKit 0x00007fff4d0001e9 WebKit::WebsiteDataStore::plugins() const + 53 2 com.apple.WebKit 0x00007fff4d000142 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>)>&&) + 2874 3 com.apple.WebKit 0x00007fff4d0006f7 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 com.apple.WebKit 0x00007fff4cfb0bf4 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 com.apple.WebKit 0x00007fff4cceee38 WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) + 8976 6 com.apple.WebKit 0x00007fff4cca3bca IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 168
Attachments
Patch (1.84 KB, patch)
2019-06-17 15:31 PDT, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Patch (3.92 KB, patch)
2019-06-18 18:03 PDT, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sihui Liu
Comment 1 2019-06-17 15:21:45 PDT
<rdar://problem/51549308>
Sihui Liu
Comment 2 2019-06-17 15:31:59 PDT
Created attachment 372279 [details] Patch
Geoffrey Garen
Comment 3 2019-06-18 10:21:05 PDT
Comment on attachment 372279 [details] Patch Separation of concerns <https://en.wikipedia.org/wiki/Separation_of_concerns> is one of our most powerful abstractions for writing maintainable code. It's why we break up code into functions, and why we break up code and data into classes. This patch eliminates some of the separation of concerns between WebsiteDataStore and WebProcessProxy because it requires WebsiteDataStore to know all of the conditions under which WebProcessProxy's processPool() might be null. If someone changes those conditions in WebProcessProxy in the future, they need to know that they should also update the code in WebsiteDataStore -- but they're very unlikely to know that since, by appearance, WebsiteDataStore and WebProcessProxy are separate classes. I'd suggest instead is that you add a function named WebProcessProxy::processPoolIfExists(). (That's not my favorite name, but it's a naming scheme we've agreed upon for WebKit.) WebProcessProxy::processPoolIfExists() should return m_processPool if it is not null, or null otherwise. If you do that, then WebsiteDataStore can call processPoolIfExists() without knowing all the conditions that might make the return value null, retaining most of the separation of concerns.
Geoffrey Garen
Comment 4 2019-06-18 10:21:22 PDT
Comment on attachment 372279 [details] Patch Oh! Also, can you write an API test for this?
Sihui Liu
Comment 5 2019-06-18 10:50:04 PDT
(In reply to Geoffrey Garen from comment #3) > Comment on attachment 372279 [details] > Patch > > Separation of concerns > <https://en.wikipedia.org/wiki/Separation_of_concerns> is one of our most > powerful abstractions for writing maintainable code. It's why we break up > code into functions, and why we break up code and data into classes. > > This patch eliminates some of the separation of concerns between > WebsiteDataStore and WebProcessProxy because it requires WebsiteDataStore to > know all of the conditions under which WebProcessProxy's processPool() might > be null. If someone changes those conditions in WebProcessProxy in the > future, they need to know that they should also update the code in > WebsiteDataStore -- but they're very unlikely to know that since, by > appearance, WebsiteDataStore and WebProcessProxy are separate classes. > > I'd suggest instead is that you add a function named > WebProcessProxy::processPoolIfExists(). (That's not my favorite name, but > it's a naming scheme we've agreed upon for WebKit.) > WebProcessProxy::processPoolIfExists() should return m_processPool if it is > not null, or null otherwise. If you do that, then WebsiteDataStore can call > processPoolIfExists() without knowing all the conditions that might make the > return value null, retaining most of the separation of concerns. Okay. Will add processPoolIfExists().
Sihui Liu
Comment 6 2019-06-18 18:03:12 PDT
Created attachment 372413 [details] Patch
Geoffrey Garen
Comment 7 2019-06-19 11:43:17 PDT
Comment on attachment 372413 [details] Patch r=me
WebKit Commit Bot
Comment 8 2019-06-19 12:44:16 PDT
Comment on attachment 372413 [details] Patch Clearing flags on attachment: 372413 Committed r246606: <https://trac.webkit.org/changeset/246606>
WebKit Commit Bot
Comment 9 2019-06-19 12:44:18 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.