Bug 100237 – IconDatabase: Properly clean up on destruction

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

[Jocelyn Turcotte]

IconDatabase: Properly cleanup on destruction

[Jocelyn Turcotte]

Created attachment 170373 [details]
Patch

[Brady Eidson]

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.

[Brady Eidson]

(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...!)

[Jocelyn Turcotte]

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.

[Brady Eidson]

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.

[Brady Eidson]

I would suggest that in your platform specific code that is run at shut down, you simply call WebIconDatabase::close()

[Jocelyn Turcotte]

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

[Brady Eidson]

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

[Jocelyn Turcotte]

Created attachment 170676 [details]
Patch

Sounds good.

[Jocelyn Turcotte]

Brady, would you have a minute to check the last patch?

[Jocelyn Turcotte]

Committed r133175: <http://trac.webkit.org/changeset/133175>