Bug 198935 – Crash at com.apple.WebKit: WebKit::WebsiteDataStore::processPools const

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

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

[Sihui Liu]

<rdar://problem/51549308>

[Sihui Liu]

Created attachment 372279 [details]
Patch

[Geoffrey Garen]

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 on attachment 372279 [details]
Patch

Oh! Also, can you write an API test for this?

[Sihui Liu]

(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]

Created attachment 372413 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372413 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 372413 [details]
Patch

Clearing flags on attachment: 372413

Committed r246606: <https://trac.webkit.org/changeset/246606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.