IconDatabase: Properly cleanup on destruction
Created attachment 170373 [details] Patch
Comment on attachment 170373 [details] Patch We don't clean up C++ objects on termination in WebCore/WebKit. The IconDatabase should be explicitly closed by each port in a way that makes sense for them.
(In reply to comment #2) > (From update of attachment 170373 [details]) > We don't clean up C++ objects on termination in WebCore/WebKit. > > The IconDatabase should be explicitly closed by each port in a way that makes sense for them. To further elaborate, the WKIconDatabaseClose() API exists to explicitly do this from the embedding application. On Mac, we want this to be explicit and not automatic. If you want it to be automatic in your port then you should put an explicit ::close() call in your port's shutdown code (assuming there is any, which there probably is...!)
Created attachment 170427 [details] Patch Ok, good to know. Tell me how this looks now, I'll do the close() in WebContext::platformInvalidateContext in another patch.
Comment on attachment 170427 [details] Patch You should only explicitly call ::close() on the WebCore::IconDatabase. You should not actually cause it to be destroyed. We don't run destructors at exit time.
I would suggest that in your platform specific code that is run at shut down, you simply call WebIconDatabase::close()
(In reply to comment #5) > We don't run destructors at exit time. The thing is that I want to allow the WebContext to be destroyed after the application destroys the last page and continues running. It's most likely not a full web browser in that case, but we still allow using the icon database. Since WebIconDatabase is refcounted and keeps the WebCore::IconDatabase in an OwnPtr, it will be destroyed with the WebContext which usually owns the last reference to the WebIconDatabase. I might be wrong somewhere so please correct me, but the only way I can see the IconDatabase not being destroyed is if the client code is expected to keep a reference outside WebKit to the WebIconDatabase or the WebContext until shutdown, and I can't enforce this. Another example is in WebIconDatabase::setDatabasePath where m_iconDatabaseImpl.clear() is called. This will also destroy the WebCore::IconDatabase and the assert will be hit.
(In reply to comment #7) > (In reply to comment #5) > > We don't run destructors at exit time. > > The thing is that I want to allow the WebContext to be destroyed after the application destroys the last page and continues running. It's most likely not a full web browser in that case, but we still allow using the icon database. > > Since WebIconDatabase is refcounted and keeps the WebCore::IconDatabase in an OwnPtr, it will be destroyed with the WebContext which usually owns the last reference to the WebIconDatabase. > I might be wrong somewhere so please correct me, but the only way I can see the IconDatabase not being destroyed is if the client code is expected to keep a reference outside WebKit to the WebIconDatabase or the WebContext until shutdown, and I can't enforce this. > > Another example is in WebIconDatabase::setDatabasePath where m_iconDatabaseImpl.clear() is called. This will also destroy the WebCore::IconDatabase and the assert will be hit. I see... interesting. In that case, the existing ASSERT is definitely invalid. But then we want to more explicitly enforce the "destruction shouldn't do meaningful cleanup" rule. Could you change the ASSERT in the IconDatabase destructor to be: ASSERT(!isOpen())
Created attachment 170676 [details] Patch Sounds good.
Brady, would you have a minute to check the last patch?
Committed r133175: <http://trac.webkit.org/changeset/133175>