User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre Build Identifier: Bug 460300 introduced an nsFaviconService::ExpireAllFavicons method which does the following: > UPDATE moz_places_view SET favicon_id = NULL WHERE favicon_id <> NULL except that I don't think that this cannot possibly work because the UPDATE trigger isn't able to set a value of NULL > "SET url = IFNULL(NEW.url, OLD.url), " \ > ... > "favicon_id = IFNULL(NEW.favicon_id, OLD.favicon_id), " \ > ... Reproducible: Always

and favicon_id <> NULL should be favicon_id NOT NULL we should expose this method in the API and wirte a unit test for it.

Shawn i can't remember why we didn't simply update both temp and disk tables.

i have an idea... We can set moz_places favicons to null before updating the view, this way the IFNULL will return null, and eventually added favicons in the meantime will be in the temp table. while there probably i'll expose this API to make it testable, that should not be late-compat because it's an add and not a change.

It is late compat because we'll have to rev the uuid on the service...

mh, well yes we have to rev the uuid, but we won't cause any uncompatibility to extensions.

it will if they have binary components

so, we can take the fix on 3.1, and the tests+idl change only on trunk, that would solve every late-compat issue still allowing us to test the code on trunk. adding privacy keyword because favicons are not expired with cache, from a favicon is possible to guess a visited site.

my previous idea does not work because we have the same trigger issue for both disk and temp table. We can't set a null value through UPDATE. I don't like this patch for 2 reasons: 1. it makes the assumption we want to remove all icons till expireAllFavicons is called... if an icon is added between the call and the queries execution, it will be retained 2. in the test i'm actually using do_timeout, and this is a possible cause of random orange. We should add again a notifier to fire a places-favicons-expired notification (like we did in autocomplete adaptive tests, this is one of those cases where i though the shared helper would have been useful) Going sync would solve both issues, but potentially locking the UI. Shawn, what do you think? Should we retain this as is, implement the notification (maybe sharing adaptive notifier), going sync? Or do you have any idea to go round the trigger?

A few comments: nsFaviconService::ExpireAllFavicons() needs to return NS_IMETHODIMP now instead of nsresult since it's on the idl javadoc comments use @note instead of note: (In reply to comment #8) > 1. it makes the assumption we want to remove all icons till expireAllFavicons > is called... if an icon is added between the call and the queries execution, it > will be retained We can have a member variable that tracks if we are expiring, and not add during that time. Use a callback from the statement when it succeeds, and flip the switch again. > 2. in the test i'm actually using do_timeout, and this is a possible cause of > random orange. We should add again a notifier to fire a places-favicons-expired > notification (like we did in autocomplete adaptive tests, this is one of those > cases where i though the shared helper would have been useful) We should just use the observer service to dispatch a notification. > Going sync would solve both issues, but potentially locking the UI. No reason to go sync - see above. With these fixes, this approach seems fine to me.

(In reply to comment #9) > > random orange. We should add again a notifier to fire a places-favicons-expired > > notification (like we did in autocomplete adaptive tests, this is one of those > > cases where i though the shared helper would have been useful) > We should just use the observer service to dispatch a notification. and to just fire the notification, we need to just have a StatementCallback object, the same thing we made for autocomplete adaptive feedback increase, so that specific notifier, now is needed again... as i said originally we need a placesStatementCallbackNotifier for these cases.

Except that we need to track and unset a variable when we complete. It'll be easier to just make our own new object here.

Looks like we were responding to the empty cache topic, but we never registered us as observers. i also fixed a compiler warning on the MAX_FAVICON_EXPIRATION const, msvc was complaining quite a bit about integer overflow. If we want to take this on 1.9.1 i'll produce a patch without the interface change and the test, just to fix the bug.

Comment on attachment 362885 [details] [diff] [review] patch v2.0 > /** >+ * Expire all known favicons from the database. >+ * @note This is an async method, on completion will fire a >+ * places-favicons-expired notification. nit: mention that this is dispatched through the observer service please >-#define MAX_FAVICON_EXPIRATION PRTime(7 * 24 * 60 * 60 * PR_USEC_PER_SEC) >+#define MAX_FAVICON_EXPIRATION ((PRTime)7 * 24 * 60 * 60 * PR_USEC_PER_SEC) I don't understand this change. The original one made it into a PRTime, and now you are casting. >+ // Same for memory-pressure notifications >+ (void)obsSvc->RemoveObserver(this, PLACES_FAVICONS_EXPIRED_TOPIC); This is not memory-pressure. Also not so sure we want to remove favicons on memory pressure since that will actually increase memory while sqlite has to do work to remove them. Favicons aren't held in memory. >@@ -205,16 +229,28 @@ nsFaviconService::Init() >+ // Same for memory-pressure notifications >+ rv = obsSvc->AddObserver(this, PLACES_FAVICONS_EXPIRED_TOPIC, PR_FALSE); >+ NS_ENSURE_SUCCESS(rv, rv); ditto > NS_IMETHODIMP > nsFaviconService::Observe(nsISupports *aSubject, const char *aTopic, > const PRUnichar *aData) > { > if (strcmp(aTopic, NS_CACHESERVICE_EMPTYCACHE_TOPIC_ID) == 0) > (void)ExpireAllFavicons(); >+ else if (strcmp(aTopic, PLACES_FAVICONS_EXPIRED_TOPIC) == 0) >+ mExpirationRunning = PR_FALSE; Ah, but you really aren't looking for memory-pressure, so the above comments are just wrong. >+NS_IMETHODIMP >+ExpireFaviconsStatementCallbackNotifier::HandleCompletion(PRUint16 aReason) >+{ >+ if (aReason != mozIStorageStatementCallback::REASON_FINISHED) >+ return NS_ERROR_UNEXPECTED; >+ >+ nsresult rv; >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService("@mozilla.org/observer-service;1", &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = observerService->NotifyObservers(nsnull, >+ PLACES_FAVICONS_EXPIRED_TOPIC, >+ nsnull); I'd prefer to just only dispatch if we have the observer service and not check rv here. Also, inconsistent naming of the observerService variable with just what you added. >diff --git a/toolkit/components/places/src/nsFaviconService.h b/toolkit/components/places/src/nsFaviconService.h >+ // Set to true during expiration, addition of new favicons won't be allowed >+ // till expiration has finished. >+ PRBool mExpirationRunning; Checked on irc. Since this is only used internally, we can use a real bool here. The bonus is that the compiler does the checking and it won't actually be an int :) >diff --git a/toolkit/components/places/tests/sync/test_database_sync_expireAllFavicons.js b/toolkit/components/places/tests/sync/test_database_sync_expireAllFavicons.js Looks good I'd like to see one more test though to make sure we clear favicons on cache clear now (couldn't test before). I really want to a test to ensure that we do not add icons when a sync is in progress, but I don't think that's possible to do.

(In reply to comment #13) > (From update of attachment 362885 [details] [diff] [review]) > >-#define MAX_FAVICON_EXPIRATION PRTime(7 * 24 * 60 * 60 * PR_USEC_PER_SEC) > >+#define MAX_FAVICON_EXPIRATION ((PRTime)7 * 24 * 60 * 60 * PR_USEC_PER_SEC) > I don't understand this change. The original one made it into a PRTime, and > now you are casting. as i told you on IRC the previous impl was causing msvc to warn about const overflow. > This is not memory-pressure. as i told you on IRC, that was a wrong comment inherited from a copy paste i did to go faster, removed > I really want to a test to ensure that we do > not add icons when a sync is in progress, but I don't think that's possible to > do. I can't see a way to do that, but i've added a test for cache::evictEntries fixed all other comments. I had to change the implementation a bit, instead of having a notifier observer now faviconsService inherits from StatementCallback, i did this because with previous implementation, in case of an error, we could end up in a situation where mExpirationRunning was hang to TRUE, ans was no more possible to add favicons. Plus our observer could be notified after other ones, with the result that if another observer was trying to add an icon when receiving the places-favicons-expired notification, mExpirationRunning was still true, and that addition was lost. That happened to me in my test for example, calling next test was failing because it was on notification. Now i always reset mRunningExpiration, so we can go on even after an error.

(In reply to comment #14) > I had to change the implementation a bit, instead of having a notifier observer > now faviconsService inherits from StatementCallback, i did this because with > previous implementation, in case of an error, we could end up in a situation > where mExpirationRunning was hang to TRUE, ans was no more possible to add > favicons. > Plus our observer could be notified after other ones, with the result that if > another observer was trying to add an icon when receiving the > places-favicons-expired notification, mExpirationRunning was still true, and > that addition was lost. > That happened to me in my test for example, calling next test was failing > because it was on notification. OK - I really don't want this object implementing the world (this is a bad programming practice IMHO), so can we just pass the pointer to the boolean to the object on creation and update it instead of making the favicon service be a callback listener please? The less an object does, the better in my book.

Comment on attachment 363897 [details] [diff] [review] patch v3.0 See comment 15

what's the current toolkit style convention for defining and passing pointers? TYPE* name or TYPE *name?

There isn't convention per se, but I think the more common style in the code base in general is TYPE *name

Comment on attachment 364081 [details] [diff] [review] patch v4.0 > /** >+ * Expire all known favicons from the database. >+ * @note This is an async method, on completion a "places-favicons-expired" >+ * notification is dispatched through observer's service. nit: need a semicolon or a period after method. >+private: >+ PRBool* mExpirationRunning; use a bool * please > nsFaviconService::nsFaviconService() : mFailedFaviconSerial(0) >+ , mExpirationRunning(PR_FALSE) and then you get to use true/false instead of PR_TRUE/PR_FALSE >+ "DELETE FROM moz_favicons WHERE id NOT IN (" >+ "SELECT favicon_id FROM moz_places_temp WHERE favicon_id NOT NULL " >+ "UNION ALL " >+ "SELECT favicon_id FROM moz_places WHERE favicon_id NOT NULL " nit: extra space before WHERE >+//////////////////////////////////////////////////////////////////////////////// >+// ExpireFaviconsStatementCallbackNotifier nit: four slashes instead of just two (see now nsIObserver is done) >+ExpireFaviconsStatementCallbackNotifier::ExpireFaviconsStatementCallbackNotifier(PRBool* aExpirationRunning) >+{ >+ mExpirationRunning = aExpirationRunning; >+} nit: bool, and please use member initialization instead of explicit assignment. add an assertion that the pointer is not null, so we don't have to check later on >+NS_IMETHODIMP >+ExpireFaviconsStatementCallbackNotifier::HandleCompletion(PRUint16 aReason) >+{ >+ if (mExpirationRunning) { >+ *mExpirationRunning = PR_FALSE; No need to check the pointer, we should be asserting if it's null >+ >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService("@mozilla.org/observer-service;1"); >+ if (observerService) { >+ (void)observerService->NotifyObservers(nsnull, >+ NS_PLACES_FAVICONS_EXPIRED_TOPIC_ID, >+ nsnull); Also, we are dispatching always regardless of the reason - should only dispatch on a successful completion. >+ // Set to true during expiration, addition of new favicons won't be allowed >+ // till expiration has finished. >+ PRBool mExpirationRunning; bool please r=sdwilsh with these fixed.

addressed comments

looks like thi sis causing crashes in browser unit tests, will investigate on that, please don't push this patch for now.

ok, was a problem in my build, rebuilding a bunch of stuff has fixed it

This bug made all my favicons disappear permanently, from bookmarks & history, everything :), using the History Submenus extension. I don't know if more extensions are affected.

Well, if that add-on clears the cache, that will likely happen.

I don't know if it does. Does this bug mean that I can never clear my cache again?

You can clear your cache, but favicons will be cleared as well.

favicons should not be cleared.

This is not a joke? Because this behavior was pre- Firefox 1, maybe even earlier. Hm, it isn't even close to 1 April. Life doesn't get easier over time. :/

This should probably be documented (both for devs and users).

It was a huge step forwards when the favicons were written to bookmarks.html as ascii, so you couldn't lose them anymore. And places was even better because also history had favicons. The problem is that you can't clear the cache anymore in case of corruptions or because of privacy/safety reasons.

clearing the favicons with the cache is awful IE6 behaviour, it will drive many users away!

AFAIK, if Firefox crashes then on start-up it clears its cache. Does the user lose their favicons under this circumstance?

Absolutely horrendous that favicons are being cleared. What is the IE6? Fix this immediately Is there another bug for this?

Hi folks, I'm gonna link to the etiquette page since some of you seem to have forgotten it: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html And then I'm going to point you to the "Depends on" list of bugs. Ye hath been warned...

I've updated the article here: https://developer.mozilla.org/En/nsIFavIconService And as a side effect have documented the Places notifications here: https://developer.mozilla.org/en/Observer_Notifications#Places Please let me know of any issues.

Re: user-doc-needed I don't really have a clear understanding of this bug. Could someone explain the steps to reproduce? I assume the doc to update is <https://support.mozilla.com/en-US/kb/Favicons+do+not+display>.

i don't think this needs more then dev-docs, there is nothing users side in this bug.