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
<rdar://problem/51549308>
Created attachment 372279 [details] Patch
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.
Comment on attachment 372279 [details] Patch Oh! Also, can you write an API test for this?
(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().
Created attachment 372413 [details] Patch
Comment on attachment 372413 [details] Patch r=me
Comment on attachment 372413 [details] Patch Clearing flags on attachment: 372413 Committed r246606: <https://trac.webkit.org/changeset/246606>
All reviewed patches have been landed. Closing bug.