1155429 - Add an explicit ImageCacheKey type for the ImageLib cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now, the ImageLib cache is keyed on URIs. That doesn't give us much flexibility - if we want to ignore certain parts of the URI, for example, or use an alternate notion of equality for some URIs (like in bug 1155332), that's quite tricky to do with this design.

Let's create an explicit key type, ImageCacheKey.

For this bug we'll retain the same behavior we have now; this is pure refactoring.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Add an explicit ImageCacheKey type for the ImageLib cache

Here's the patch.

I've taken pains to ensure that we never create an ImageCacheKey (and hence
never hash a URI) unless we actually need the hash, since that's not free.
That's why I converted a few functions to just take |aForChrome| arguments,
since that's really all the information they needed.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Sorry, bad copy/paste on my part. When I mentioned bug 1155332 in comment 0, I meant bug 1154974.

Comment 3

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9176ca23ba2a

Comment 4

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1fc6d393500

Comment 5

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c4d9e3c18e

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8593650 [details] [diff] [review]
Add an explicit ImageCacheKey type for the ImageLib cache

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

::: image/src/imgLoader.cpp
@@ +1098,5 @@
> +// ImageCacheKey
> +///////////////////////////////////////////////////////////////////////////////
> +
> +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +1100,5 @@
> +
> +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> +{
> +  bool isChrome;
> +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;

same comment about resource://

@@ +1107,5 @@
> +  mHash = ComputeHash(mSpec);
> +}
> +
> +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +1109,5 @@
> +
> +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> +{
> +  bool isChrome;
> +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;

here too.

::: image/src/imgLoader.h
@@ +219,5 @@
> + *
> + * We key the cache on the initial URI (before any redirects), with some
> + * canonicalization applied. See ComputeHash() for the details.
> + */
> +class ImageCacheKey final

It seems to be MOZ_STACK_CLASS only, right?

@@ +240,5 @@
> +
> +private:
> +  static uint32_t ComputeHash(const nsAutoCString& aSpec);
> +
> +  nsAutoCString mSpec;

nsAutoCStrring should be used just for stack-based string. Should it be nsCString instead... or add MOZ_STACK_CLASS.

::: image/src/imgRequest.cpp
@@ +457,5 @@
> +bool
> +imgRequest::IsChrome() const
> +{
> +  bool isChrome = false;
> +  if (NS_FAILED(mURI->SchemeIs("chrome", &isChrome))) {

NS_WARN_IF(NS_FAILED( ...

@@ +459,5 @@
> +{
> +  bool isChrome = false;
> +  if (NS_FAILED(mURI->SchemeIs("chrome", &isChrome))) {
> +    return false;
> +  }

often we use resource:// in addons, jetpack and other stuff.

Comment 7

[Andrea Marchesini [:baku]]

ah... the chrome vs resource seems to be a preexisting issue. Maybe can be a follow up if you think it makes sense.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! I'll make most of those changes; a couple of responses below.

(In reply to Andrea Marchesini (:baku) from comment #6)
> @@ +1100,5 @@
> > +
> > +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> > +{
> > +  bool isChrome;
> > +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;
> 
> same comment about resource://

As you mentioned in comment 7, I'm just maintaining how things work right now: only chrome:// stuff goes in the chrome cache. I can see a good case that resource:// should go in there as well, though. We can make that change in a followup.

> > +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> > +{
> 
> MOZ_ASSERT(NS_IsMainThread());

An ImageURL is safe to use off-main-thread - that's actually the reason ImageURL exists. Agreed about the nsIURI constructor, though.

> It seems to be MOZ_STACK_CLASS only, right?

Nah, we store these in the hash table.

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=837acd99eaf4

Comment 10

[Seth Fowler [:seth] [:s2h]]

I filed bug 1159084 about the resource:// URI issue.

Also, I finally figured out the bug that was causing those test failures on try: nsAutoCString cannot be memmove'd, and therefore cannot be stored in many of our data structures. Sigh. Lesson learned.

I'll upload a revised version of the patch shortly that addresses the review comments, and then I think this might be ready to go.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: Add an explicit ImageCacheKey type for the ImageLib cache

OK, here's the final version of the patch.

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffef3cb1aa62

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03f05d08f718

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03f05d08f718