[Webkit-unassigned] [Bug 98508] [WK2] Icons are retained but never released

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98508





--- Comment #16 from Brady Eidson <beidson at apple.com>  2012-10-17 09:35:32 PST ---
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > (In reply to comment #9)
> > > > > > (In reply to comment #8)
> > > > > > > (In reply to comment #7)
> > > > > > > > The design for this scenario is that unretained icons get cleaned up on the next launch.  That's where something is going wrong here, around step 4 + 5.
> > > > > > > > 
> > > > > > > > Whats the delayDatabaseCleanup/allowDatabaseCleanup situation on the next launch?
> > > > > > > 
> > > > > > > WebIconDatabase disables the database cleanup before opening the database, and it's never enabled again. In WebKit1, we enable database cleanup after the URL import, but all icons are added to m_pageURLToRecordMap during the URL import when the database clean up is delayed, so previous unretained icons won't be pruned in this case either.
> > > > > > 
> > > > > > It's not possible for WebKit itself to know when it is appropriate to enable database cleanup, so re-enabling database cleanup is exposed as API.
> > > > > > 
> > > > > > The browser application itself has to make those decisions.  For example, the browser might take ~1 minute to lazily load all URLs for its history and bookmarks and it wishes to retain the icon for each of those URLs.  Once it's done doing that it calls the WKIconDatabaseEnableDatabaseCleanup API.
> > > > > 
> > > > > This is exactly what the GTK+ WebKit1 API does, when didFinishURLImport() is called the database cleanup is enabled. Then, the icons we want to keep in the database are retained and never released. 
> > > > 
> > > > What didFinishURLImport() are you talking about, here?  Is that a GTK API concept?
> > > 
> > > No, the IconDatabase client callback to notify the user of the icon database that the URLs have been imported.
> > > 
> > > > Because the only didFinishURLImport() I know about is Internal to WebCore and has nothing to do with the IconDatabase.  didFinishURLImport() is called when the IconDatabase itself is done loading URLs from the database on disk, but it has *nothing* to do with whether or not the browser application has finished retaining all URLs it is interested in.
> > > 
> > > Yes, but the URL import ends up retaining all existing icons in the database in practice.
> > 
> > > 1- The database cleanup is delayed
> > 
> > Correct.  While we're opening the database, nothing should be eligible to be cleaned up.
> > 
> > > 2- The database is opened
> > 
> > Correct.  And since database cleanup is delayed, we can just cruise through it importing URLs.
> > 
> > > 3- The URL import starts. It reads all the page URLs in the database and since cleanup is delayed all of them are added to m_pageURLToRecordMap. 
> > 
> > Correct.  This is by design.  We read every URL in before anything is allowed to be pruned.
> > 
> > > 4- When pruning icons, all existing icons at startup are in m_pageURLToRecordMap and new icons released are not pruned because they are still retained by the history items.
> > 
> > Actually, no.
> > 
> > Where do these history items you speak of come from?  In Safari, for example, Safari retains each URL in its browser history.  But nothing in WebCore/WebKit automatically makes these history items.
> > 
> 
> I'm talking about Source/WebCore/history/HistoryItem.cpp that retains the URL automatically in the constructor (or when a URL is set) and released it in the destructor. These items are created automatically by the history controller and frame loader when loading or navigating pages. History items are kept cached until the web process finishes, when the destructor of the items releases the URL but it's too late for the UI process to get notified.

Right, but on the *next launch* - with a fresh WebProcess - these HistoryItems don't exist.  Right after you launch a browser to a single blank window there is no back/forward history.

Therefore the URLs they had pointed too last time *are* eligible to be pruned, unless the client retains them for some other reason.

> > Remember that "delayDatabaseCleanup" from step 1?  It hasn't been balanced by an "allowDatabaseCleanup" yet.  Nothing will ever get pruned until allowDatabaseCleanup is called.
> 
> I'm assuming the application enables the database cleanup after the URL import is completed.

No.  The URL import is an internal implementation detail of the IconDatabase.  If you're look at WebKit code then you know that the IconDatabase won't do any pruning until after the URL import is complete.  But from the *outside*, all you need to know is "the IconDatabase won't do any pruning until after I call WKIconDatabaseEnableDatabaseCleanup.  It might do it right then, or it might do it 10 minutes later, but I need to call WKIconDatabaseEnableDatabaseCleanup before pruning is even allowed."

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.