460300 - Expire favicons when they expire from cache, and when the cache is cleared

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

Right now we expire favicons every 24 hours so we can pick up changes or when the cache is cleared.  We end up doing a lot of writes for favicons because of this, so we should move out the expiration.

This code used to grab the expiration time from the cache which might even be a better solution as long as we clear when the cache is cleared.

Comment 1

[John P Baker]

For data: icons this should probably be a much longer expiration time; We don't make much use of them yet - except in my extension - but, depending on bug 423126, this could increase.

Comment 2

[Shawn Wilsher :sdwilsh]

For reference, the old code that used to grab the expiration time can be found here:
https://bugzilla.mozilla.org/attachment.cgi?id=213906&action=diff#mozilla/browser/components/places/src/nsFaviconService.cpp_sec11

(In reply to comment #1)
> For data: icons this should probably be a much longer expiration time; We don't
> make much use of them yet - except in my extension - but, depending on bug
> 423126, this could increase.
I think we should probably do that in a follow-up bug

Comment 3

[Shawn Wilsher :sdwilsh]

Requesting blocking.  We can greatly reduce the number of times we write out to moz_favicons (part of places.sqlite) and moz_places with this change.  This will need bug 462173 because we use the multiple statement API.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 348082 [details] [diff] [review]
v1.0

This makes us expire when the cache tells us it will expire, capped at one week, or in one week if we can't get a time from the cache (or it is in the past).  It also clears all favicons when the cache is cleared.

Comment 6

[Marco Bonardo [:mak]]

+  // Remove all references to favicons
+  nsCOMPtr<mozIStorageStatement> removeReferences;
+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+      "UPDATE moz_places_view "
+      "SET favicon_id = NULL"
+    ), getter_AddRefs(removeReferences));
+

this could potentially copy all places from memory to disk due to the trigger, should have a where cause, but selecting directly on the view could be slow.

Comment 7

[Marco Bonardo [:mak]]

also, add error checking inside expireAllFavicons

and, do we really have to get expiration values at every call to SetFaviconUrlForPage?

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 348082 [details] [diff] [review]
v1.0

approach looks fine, cancelling review so you can address marco's comments.

Comment 9

[Dietrich Ayala (:dietrich)]

Yes this will block, as it's causing disk writes on a per-page-load basis.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> this could potentially copy all places from memory to disk due to the trigger,
> should have a where cause, but selecting directly on the view could be slow.
Yes, but how often does a user clear the cache?  I think we'll be OK for common cases.

(In reply to comment #7)
> and, do we really have to get expiration values at every call to
> SetFaviconUrlForPage?
Turns out nothing actually cared about that.  I'll also note that that is a poor name for that method, but I don't want to rename it in this patch...

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> this could potentially copy all places from memory to disk due to the trigger,
> should have a where cause, but selecting directly on the view could be slow.
Also, if we have embed visists in memory, the amount of pages we have in our database is much much smaller.  We could guard on type if we wanted to here.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

my bad - I got another patch with this one too...

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Addresses comments as discussed on irc.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 350878 [details] [diff] [review]
v1.2

> 
>+/**
>+ * The maximum time we will keep a favicon around.  We always ask the cache, if
>+ * we can, but default to this value if we do not get a time back, or the time
>+ * is more in the future than this.
>+ * Currently set to one week from now.
>+ */
>+#define MAX_FAVICON_EXPIRATION PRInt64(7 * 24 * 60 * 60 * PR_USEC_PER_SEC)

should not this be a PRTime? that's a PRInt64 but since this is a time i suppose it would be better to be consistent.

> class FaviconLoadListener : public nsIStreamListener,
>                             public nsIInterfaceRequestor,
>                             public nsIChannelEventSink
> {
> 
>-  nsCOMPtr<nsFaviconService> mFaviconService;
>+  nsRefPtr<nsFaviconService> mFaviconService;

is this change needed for multiple inheritance?

>+
>+  mozIStorageStatement *stmts[] = {
>+    removeReferences,
>+    removeFavicons
>+  };
>+  nsCOMPtr<mozIStoragePendingStatement> ps;
>+  return mDBConn->ExecuteAsync(stmts, NS_ARRAY_LENGTH(stmts), nsnull,
>+                               getter_AddRefs(ps));
>+}

we usually NS_ENSURE_SUCCESS and return NS_OK to get exception at the correct point.

>+  // Attempt to get an expiration time from the cache.  If this fails, we'll
>+  // make one up.
>+  PRTime expiration = -1;
>+  nsCOMPtr<nsICachingChannel> cachingChannel(do_QueryInterface(mChannel));
>+  if (cachingChannel) {
>+    nsCOMPtr<nsISupports> cacheToken;
>+    rv = cachingChannel->GetCacheToken(getter_AddRefs(cacheToken));
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsICacheEntryInfo> cacheEntry(do_QueryInterface(cacheToken));
>+      PRUint32 seconds;
>+      rv = cacheEntry->GetExpirationTime(&seconds);
>+      if (NS_SUCCEEDED(rv)) {
>+        // Set the expiration, but make sure we honor our cap.
>+        expiration = PR_Now() + PR_MIN(seconds * PR_USEC_PER_SEC,
>+                                       MAX_FAVICON_EXPIRATION);

cast seconds to PRTime

I don't know well the cacheService, would it be possible to write a test that sets the cache expiration time to a low value and check that favicon expiration is correct? OR alternatively are there litmus steps to verify this works correctly?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> is this change needed for multiple inheritance?
yes

> I don't know well the cacheService, would it be possible to write a test that
> sets the cache expiration time to a low value and check that favicon expiration
> is correct? OR alternatively are there litmus steps to verify this works
> correctly?
I thought about writing a test, but I'm quite certain it'd take several days to write given the amount of code I'd have to do for the setup.

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Addresses review comments

Comment 18

[Dietrich Ayala (:dietrich)]

please provide litmus steps if an automated test is not feasible.

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> please provide litmus steps if an automated test is not feasible.
I'm not even sure if that is feasible.  You have three situations, two of which may be testable:
1) Server has a favicon set to expire in short amount of time (let's say five minutes), load it, and change it on the server, and load it again in five minutes and make sure it changes.
2) Server has a favicon that expires some time in the future.  Load the page with it, change it, and come back in a week to make sure it updates.
3) Load a page with a favicon, change it, clear the cache, reload it.  It should change.

Comment 20

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/005a32ae9d7a

Comment 21

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b290a7c0c0d0

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b290a7c0c0d0
I totally landed this before I landed bug 462173 on branch (it fell through my radar).  We might get some strange bugs on today's nightly because of it.