Bug 98508 – [WK2] Icons are retained but never released

• Status: NEW
• Component: WebKit2

[Carlos Garcia Campos]

The problem is that HistoryItem retains icons for the page URL on construction and releases then on destruction. This happens in the Webrocess so a message is sent to the UI process. When history items are deleted, is usually because the WebProcess finishes and quite often the UI process has already finished or has exited the main loop, so the message never gets to the UI process and all icons are always retained.

[Carlos Garcia Campos]

Since the icon database is in the ui process, maybe we could retain/release icons for the history items in the ui process when WebBackForwardListItem is created/destroyed by the ui process, and remove the messages to retain/release icons from the web process.

[Mario Sanchez Prada]

(In reply to comment #1)
> Since the icon database is in the ui process, maybe we could retain/release 
> icons for the history items in the ui process when WebBackForwardListItem is 
> created/destroyed by the ui process, and remove the messages to retain/release 
> icons from the web process.

I agree with this. Not sure if there's any specific limitation to retain/release icons in the UI process, but if there is not it certainly sounds to me like a good idea to do it in the same place where the IconDatabase actually lives.

[Brady Eidson]

(In reply to comment #0)
> The problem is that HistoryItem retains icons for the page URL on construction and releases then on destruction. This happens in the Webrocess so a message is sent to the UI process. When history items are deleted, is usually because the WebProcess finishes and quite often the UI process has already finished or has exited the main loop, so the message never gets to the UI process and all icons are always retained.

Is this a problem in practice, though?  Because aren't the appropriate retain counts set up again the next time the browser launches?

[Carlos Garcia Campos]

The problem in practice is that you end up with favicons in the database that you don't want, not a big issue, of course.

[Brady Eidson]

(In reply to comment #4)
> The problem in practice is that you end up with favicons in the database that you don't want, not a big issue, of course.

I'm confused.  Can you more explicitly spell out how - after the database pruning on the next launch - you would end up with icons you don't want?

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > The problem in practice is that you end up with favicons in the database that you don't want, not a big issue, of course.
> 
> I'm confused.  Can you more explicitly spell out how - after the database pruning on the next launch - you would end up with icons you don't want?

hmm, what icons are pruned on next launch? this is the test I have done:

1.- Delete the database
2.- Load two pages in MiniBrowser and retain the icon for one but release for the other
3.- Close MiniBrowser, both icons are in the database.
4.- Open MiniBrowser and load only the page that I retained the icon, to make sure I don't retain the released icon.
5.- Close MiniBrowser, both icons are in the database.

Without retaining/releasing the icons in HistoryItem, the released icon is not in the database when I close MiniBrowser.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > The problem in practice is that you end up with favicons in the database that you don't want, not a big issue, of course.
> > 
> > I'm confused.  Can you more explicitly spell out how - after the database pruning on the next launch - you would end up with icons you don't want?
> 
> hmm, what icons are pruned on next launch? this is the test I have done:
> 
> 1.- Delete the database
> 2.- Load two pages in MiniBrowser and retain the icon for one but release for the other
> 3.- Close MiniBrowser, both icons are in the database.
> 4.- Open MiniBrowser and load only the page that I retained the icon, to make sure I don't retain the released icon.
> 5.- Close MiniBrowser, both icons are in the database.
> 
> Without retaining/releasing the icons in HistoryItem, the released icon is not in the database when I close MiniBrowser.

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?

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > The problem in practice is that you end up with favicons in the database that you don't want, not a big issue, of course.
> > > 
> > > I'm confused.  Can you more explicitly spell out how - after the database pruning on the next launch - you would end up with icons you don't want?
> > 
> > hmm, what icons are pruned on next launch? this is the test I have done:
> > 
> > 1.- Delete the database
> > 2.- Load two pages in MiniBrowser and retain the icon for one but release for the other
> > 3.- Close MiniBrowser, both icons are in the database.
> > 4.- Open MiniBrowser and load only the page that I retained the icon, to make sure I don't retain the released icon.
> > 5.- Close MiniBrowser, both icons are in the database.
> > 
> > Without retaining/releasing the icons in HistoryItem, the released icon is not in the database when I close MiniBrowser.
> 
> 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.

[Brady Eidson]

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

[Carlos Garcia Campos]

(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. We could do the same in webkit2, I guess we need a callback in the C API client to notify when URL import has finished. This won't prune previous unretained icons (because it's impossible to know which icons were released in a previous run) but would allow to prune icons not used for 30 days (this is GTK+ specific)

[Brady Eidson]

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

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.

>We could do the same in webkit2, I guess we need a callback in the C API client to notify when URL import has finished. This won't prune previous unretained icons (because it's impossible to know which icons were released in a previous run) but would allow to prune icons not used for 30 days (this is GTK+ specific)

This comment makes me think that you're still not quite groking the design of the IconDatabase mechanism *or* there's some design within GTK that is at odds with how the database is meant to work.

[Brady Eidson]

(In reply to comment #11)

> Because the only didFinishURLImport() I know about is Internal to WebCore and has nothing to do with the IconDatabase. 

I meant "...and has noting to do with the IconDatabase API"

[Carlos Garcia Campos]

(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
2- The database is opened
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. 
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.

> >We could do the same in webkit2, I guess we need a callback in the C API client to notify when URL import has finished. This won't prune previous unretained icons (because it's impossible to know which icons were released in a previous run) but would allow to prune icons not used for 30 days (this is GTK+ specific)
> 
> This comment makes me think that you're still not quite groking the design of the IconDatabase mechanism *or* there's some design within GTK that is at odds with how the database is meant to work.

Maybe, thanks for your help with this by the way. Is it documented somewhere? We are using the icon database as a permanent cache of favicons. Our API wraps the icon database to provide a high level API to ask for icons. By default we want to retain all icons loaded, so that they are stored in the database, but when something fails getting the icon or the web page doesn't have a favicon, we release the icon to not store it in the database.

[Brady Eidson]

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

Remember that "delayDatabaseCleanup" from step 1?  It hasn't been balanced by an "allowDatabaseCleanup" yet.  Nothing will ever get pruned until allowDatabaseCleanup is called.

In Safari, for example, it lazily retains all the URLs in its browser history and bookmarks knowing that the database won't prune until it's told to.  After it's done importing history/bookmarks, it calls WKIconDatabaseEnableDatabaseCleanup.

I suspect the real reason you're seeing nothing get pruned is because nothing in your GTK application is calling "WKIconDatabaseEnableDatabaseCleanup" to signal the database that the application is done retaining URLs.

Does this make sense?

[Carlos Garcia Campos]

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

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

> In Safari, for example, it lazily retains all the URLs in its browser history and bookmarks knowing that the database won't prune until it's told to.  After it's done importing history/bookmarks, it calls WKIconDatabaseEnableDatabaseCleanup.

I see.

> I suspect the real reason you're seeing nothing get pruned is because nothing in your GTK application is calling "WKIconDatabaseEnableDatabaseCleanup" to signal the database that the application is done retaining URLs.
> 
> Does this make sense?

I'll do more tests to confirm it, because I tried it enabling the database cleanup right after the URL import finishes.

[Brady Eidson]

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