476636 - nsFaviconService::ExpireAllFavicons cannot work

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[John P Baker]

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

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 4

[Shawn Wilsher :sdwilsh]

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Shawn Wilsher :sdwilsh]

it will if they have binary components

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

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?

Comment 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 11

[Shawn Wilsher :sdwilsh]

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.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.0

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 13

[Shawn Wilsher :sdwilsh]

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.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v3.0

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

Comment 15

[Shawn Wilsher :sdwilsh]

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

[Shawn Wilsher :sdwilsh]

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

See comment 15

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v4.0

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

Comment 18

[Shawn Wilsher :sdwilsh]

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

Comment 19

[Shawn Wilsher :sdwilsh]

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.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v4.1

addressed comments

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/2a85f066e124

Comment 24

[Ria Klaassen (not reading all bugmail)]

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.

Comment 25

[Shawn Wilsher :sdwilsh]

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

Comment 26

[Ria Klaassen (not reading all bugmail)]

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

Comment 27

[Shawn Wilsher :sdwilsh]

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

Comment 28

[pal-moz]

favicons should not be cleared.

Comment 29

[Ria Klaassen (not reading all bugmail)]

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

Comment 30

[Nochum Sossonko [:Natch]]

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

Comment 31

[Ria Klaassen (not reading all bugmail)]

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.

Comment 32

[Barend]

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

Comment 33

[Steve England [:stevee]]

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

Comment 34

[John]

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

Comment 35

[Shawn Wilsher :sdwilsh]

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

Comment 36

[Eric Shepherd [:sheppy]]

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.

Comment 37

[Chris Ilias [:cilias]]

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

Comment 38

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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