Closed Bug 1352408 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: ImageLib, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: tnikkel)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(tnikkel)
How are you loading the images? Calling imgLoader::LoadImage?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
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
The protocol implementation is in PageIconProtocolHandler.js, it is slightly modified by bug 977177 but the basics are not changing.
ehr, in comment 3, when I say refresh I actually mean "mark as invalid".
You don't need elements to have their image updated without setting their src again do you?
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)?
(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.
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?
Flags: needinfo?(ehsan)
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.
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.
(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.
Flags: needinfo?(ehsan)
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.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel) → needinfo?(mak77)
Attachment #8855955 - Flags: review?(ehsan)
noscript is fine.
Flags: needinfo?(mak77)
(and no, uuids don't need to be updated anymore)
Comment on attachment 8855955 [details] [diff] [review]
restore removeentry

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

::: image/imgICache.idl
@@ +18,5 @@
>   * @author Stuart Parmenter <pavlov@netscape.com>
>   * @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.
Attachment #8855955 - Flags: review?(ehsan) → review+
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e2627b287
Restore imgICache::RemoveEntry. r=ehsan
Thank you for the help.
https://hg.mozilla.org/mozilla-central/rev/f52e2627b287
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.