1352408 - Need a way to bypass image cache for certain loads or scheme

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Marco Bonardo [:mak]]

For new favicons I have a page-icon:page_url protocol that returns the best icon for a given url.
I'm using these urls in the ui to show favicons for the various nodes (bookmarks toolbar, menu library and so on).
The problem is that when a page is reloaded, the icon may change (maybe from the default one to the proper icon from the page) and then I need to change the icon in the UI.
I can't find a way to do that, the image cache keeps noticing the image url is the same and doesn't even start a new load.
I wonder if there's a way to bypass the cache for a specific schema, load, or clear the cache for a url when I know it's invalidated.
Fwiw, this sounds a lot like https://bugzilla.mozilla.org/show_bug.cgi?id=1202085#c42, but at least I'm on a custom protocol that gives me some sandboxing from global changes to the image cache.

Ideally I'd still like to use the image cache (so if a view has ten identical favicons we don't load all of them), but I really need a way to force a cache refresh when an icon changes.

I wonder if there's any way the cache could read some caching information from the image channel, for example if the channel has INHIBIT_CACHING or LOAD_BYPASS_CACHE.

Any ideas are very welcome since this is a show stopper for me, but I'd prefer avoiding fancy things like appending random query strings to my url.

Comment 1

[Timothy Nikkel (:tnikkel)]

How are you loading the images? Calling imgLoader::LoadImage?

Comment 2

[Marco Bonardo [:mak]]

No, I'm just setting the "image" attribute into xul elements (menuitems, menus, also trees), that is likely changing list-style-image or src.
Off-hand I can't predict on which elements this may be used in the future, for example Activity Stream may use icons in their html UI.
My protocol just fetches payloads from the db and streams them out through a pipe with the proper content type.

Comment 3

[Marco Bonardo [:mak]]

The only alternatives I see atm are:
1. completely blacklist this scheme from the cache. This is bad from a perf point of view.
2. have a way to tell to the cache that a certain url is now invalid
3. have a way to tell to the cache to refresh all urls with my scheme

Hope you may have better ideas.
In case you can fetch debug Try builds from this run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2fd5b4af803426d9d116d34f50e3f08ea6aa1e

Comment 4

[Marco Bonardo [:mak]]

The protocol implementation is in PageIconProtocolHandler.js, it is slightly modified by bug 977177 but the basics are not changing.

Comment 5

[Marco Bonardo [:mak]]

ehr, in comment 3, when I say refresh I actually mean "mark as invalid".

Comment 6

[Timothy Nikkel (:tnikkel)]

You don't need elements to have their image updated without setting their src again do you?

Comment 7

[Timothy Nikkel (:tnikkel)]

By that I mean that for any elements whose visual display you want to update you mutate the element so that it at least asks the image cache for an image (even if the image cache is returning the old image with the existing code)?

Comment 8

[Marco Bonardo [:mak]]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> You don't need elements to have their image updated without setting their
> src again do you?

nope, I know when the image changes, I have a notification. I don't expect the image to magically change by itself.

Comment 9

[Timothy Nikkel (:tnikkel)]

Ehsan, what was the reason RemoveEntry was removed in bug 1202085? Can we just provide an updated version of RemoveEntry with the addition of a document argument?

Comment 10

[Marco Bonardo [:mak]]

I was able to confirm that the problem is with the image cache, removing the specific entry from it when I know the image is about to be invalidated solves it.

Comment 11

[Marco Bonardo [:mak]]

Sorry for nagging, but this a priority for us, we would like to have enough testing time in Nightly before the next merge and all this work is planned for 55.

Comment 12

[(no longer active)]

(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> Ehsan, what was the reason RemoveEntry was removed in bug 1202085? Can we
> just provide an updated version of RemoveEntry with the addition of a
> document argument?

Sorry for my delay in responding here.

The reason was this part of the patch set <https://hg.mozilla.org/integration/mozilla-inbound/rev/afc27ae3cc45> where we added the pointer to the controlled document to the hash key for image cache entries, so the previous API needed to be changed.  IIRC there wasn't any consumers in the codebase so after some IRC discussion (around bug 1202085 comment 18) we agreed that removing the API is fine.  I think there was some rough agreement that if we needed to bring it back again we just needed to add a document argument as you suggest here, so doing that now should be OK.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: restore removeentry

I'm not sure if uuids need to be updated at all anymore?

I made the function noscript because the snippet you showed me was C++. Do you need this to be scriptable? If not I'd prefer to have noscript because it's easy to make it scriptable in the future, harder to remove a scriptable interface.

Comment 14

[Marco Bonardo [:mak]]

noscript is fine.

Comment 15

[Marco Bonardo [:mak]]

(and no, uuids don't need to be updated anymore)

Comment 16

[(no longer active)]

Comment on attachment 8855955 [details] [diff] [review]
restore removeentry

Review of attachment 8855955 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/imgICache.idl
@@ +18,5 @@
>   * @author Stuart Parmenter <[e-mail]>
>   * @version 0.1
>   * @see imagelib2
>   */
> +[scriptable, builtinclass, uuid(9bfedca8-b20f-4427-aa65-1e9648753e6f)]

This isn't needed as Marco said but doesn't hurt either!

::: image/imgLoader.cpp
@@ +1374,5 @@
>  }
>  
>  NS_IMETHODIMP
> +imgLoader::RemoveEntry(nsIURI* aURI,
> +                               nsIDOMDocument* aDOMDoc)

Nit: indentation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e2627b287
Restore imgICache::RemoveEntry. r=ehsan

Comment 18

[Marco Bonardo [:mak]]

Thank you for the help.

Comment 19

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/f52e2627b287