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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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

9 years ago
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.
(Assignee)

Updated

9 years ago
Summary: Expire favicons once a week, and when the cache is cleared → Expire favicons when they expire from cache, and when the cache is cleared
(Assignee)

Comment 2

9 years ago
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
(Assignee)

Comment 3

9 years ago
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.
Depends on: 462173
Flags: blocking1.9.1?
Target Milestone: --- → mozilla1.9.1b1
(Assignee)

Comment 5

9 years ago
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.
Attachment #348082 - Flags: review?(dietrich)
+  // 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.
also, add error checking inside expireAllFavicons

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

Updated

9 years ago
Blocks: 403650
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review dietrich]
Comment on attachment 348082 [details] [diff] [review]
v1.0

approach looks fine, cancelling review so you can address marco's comments.
Attachment #348082 - Flags: review?(dietrich)
Yes this will block, as it's causing disk writes on a per-page-load basis.
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [has patch][needs review dietrich] → [has patch][needs new patch]
Priority: -- → P1
Whiteboard: [has patch][needs new patch] → [needs new patch]
(Assignee)

Comment 10

9 years ago
(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...
(Assignee)

Comment 11

9 years ago
(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.
(Assignee)

Comment 12

9 years ago
Created attachment 350870 [details] [diff] [review]
v1.1
Attachment #348082 - Attachment is obsolete: true
Attachment #350870 - Flags: review?(mak77)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch] → [has patch][needs review Marco]
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1b3
(Assignee)

Comment 13

9 years ago
Created attachment 350872 [details] [diff] [review]
v1.1

my bad - I got another patch with this one too...
Attachment #350870 - Attachment is obsolete: true
Attachment #350872 - Flags: review?(mak77)
Attachment #350870 - Flags: review?(mak77)
(Assignee)

Comment 14

9 years ago
Created attachment 350878 [details] [diff] [review]
v1.2

Addresses comments as discussed on irc.
Attachment #350872 - Attachment is obsolete: true
Attachment #350878 - Flags: review?(mak77)
Attachment #350872 - Flags: review?(mak77)
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?
Attachment #350878 - Flags: review?(mak77) → review+

Updated

9 years ago
Whiteboard: [has patch][needs review Marco] → [has patch]
(Assignee)

Comment 16

9 years ago
(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.
(Assignee)

Comment 17

9 years ago
Created attachment 351077 [details] [diff] [review]
v1.3

Addresses review comments
Attachment #350878 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: [has patch] → [has patch][has review][can land]
please provide litmus steps if an automated test is not feasible.
(Assignee)

Comment 19

9 years ago
(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.
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/005a32ae9d7a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
(Assignee)

Comment 22

9 years ago
(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.

Updated

9 years ago
Depends on: 476636

Updated

9 years ago
Depends on: 480873
You need to log in before you can comment on or make changes to this bug.