Closed Bug 1160703 Opened 5 years ago Closed 5 years ago

Associate an imgRequest with its ImageCacheKey

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

In bug 1154974, it became clear that reconstructing the ImageCacheKey from an imgRequest's URI every time is a problem. Doing that means that, once the patches in bug 1154974 land, the ImageCacheKey we get for blob URIs can change depending on whether revokeObjectUrl() has been called for that blob URI. That means, in turn, that we can find it difficult to remove an imgRequest from the image cache, because we can no longer reconstruct the ImageCacheKey it's stored with.

We should fix the design to avoid this problem. In this bug, I'll associate an imgRequest with its ImageCacheKey, to ensure that we always use the same one when looking it up in the image cache.
OK, in this part we just move ImageCacheKey out of imgLoader.h. We need to do
this because later on we run into mutual include issues if we don't, because
imgRequest.h starts using ImageCacheKey in part 3.
Attachment #8600542 - Flags: review?(amarchesini)
In this part we start storing an ImageURL instead of a URI spec string in
ImageCacheKey. This is to reduce copying, since after part 3 we will store each
ImageCacheKey twice for each imgRequest - once as the key in the image cache
hashtable, and once on the imgRequest object itself. If we use the same ImageURL
for both, this doesn't cost much, so we switch to that approach in this patch.
Attachment #8600543 - Flags: review?(amarchesini)
In this part, we add an ImageCacheKey member to imgRequest, which we pass in via
the constructor. This patch also makes the code in imgLoader use the
imgRequest's ImageCacheKey whenever possible; we avoid ever constructing a new
one from the URI, as we used to do.

This should both be cheaper (less copying and re-hashing) and should avoid the
issues that showed up on try in bug 1154974.
Attachment #8600544 - Flags: review?(amarchesini)
Comment on attachment 8600544 [details] [diff] [review]
(Part 3) - Associate an imgRequest with its ImageCacheKey

Unsetting review because it seems there's a bug here. I'm pretty sure it's in part 3, but I'll push another try job to verify that.
Attachment #8600544 - Flags: review?(amarchesini)
Yep, looks like parts 1 and 2 by themselves are green.
Comment on attachment 8600542 [details] [diff] [review]
(Part 1) - Move ImageCacheKey out of imgLoader.h

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

::: image/src/ImageCacheKey.h
@@ +9,5 @@
> +
> +#ifndef mozilla_image_src_ImageCacheKey_h
> +#define mozilla_image_src_ImageCacheKey_h
> +
> +#include "mozilla/Maybe.h"

you don't need this.
Attachment #8600542 - Flags: review?(amarchesini) → review+
Comment on attachment 8600543 [details] [diff] [review]
(Part 2) - Store an ImageURL pointer instead of a URI spec string in ImageCacheKey

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

::: image/src/ImageCacheKey.cpp
@@ +36,1 @@
>  

MOZ_ASSERT(NS_IsMainThread()); also here?

@@ +45,3 @@
>    , mHash(aOther.mHash)
>    , mIsChrome(aOther.mIsChrome)
>  { }

MOZ_ASSERT(NS_IsMainThread());

@@ +51,3 @@
>    , mHash(aOther.mHash)
>    , mIsChrome(aOther.mIsChrome)
>  { }

MOZ_ASSERT(NS_IsMainThread());
Attachment #8600543 - Flags: review?(amarchesini) → review+
Depends on: 1163866
Thanks for the review! I've rebased part 1 and updated it according to the review comments.
Thanks for the review! I've rebased part 2.

It might not be clear, but we need to be able to perform basic operations on
ImageCacheKey objects off-main-thread, so I didn't add those assertions.

The main thing we want to be sure not to do is use nsIURI objects
off-main-thread, as they aren't thread safe. We have assertions for those cases.
ImageURL objects are threadsafe, so we don't need to assert when we're only
dealing with them.
OK, here's a working version of part 3. It's basically the same as the version
that I originally posted, but it's been rebased and I made a couple of minor
tweaks. Most importantly, I added some new assertions, which helped me finally
figure out what was causing the test failures from this patch. The cause was bug
1163866.

Now that that's resolved, this patch is green on try, and it's finally ready to
be reviewed.
Attachment #8605677 - Flags: review?(amarchesini)
Comment on attachment 8605677 [details] [diff] [review]
(Part 3) - Associate an imgRequest with its ImageCacheKey

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

lgtm
Attachment #8605677 - Flags: review?(amarchesini) → review+
Thanks for the review! I think this is finally ready to land. Whew.
Oh dear. =( Another test failure seems to have sprouted since my try run 5 days ago. Investigating...
Attachment #8600542 - Attachment is obsolete: true
Attachment #8600543 - Attachment is obsolete: true
Attachment #8600544 - Attachment is obsolete: true
Alright, I now think there is pretty good reason to suspect another Necko-level bug here.

I'm going to pull the assertion out of this bug and move it to a separate bug, so landing it can be blocked on the Necko work.
Blocks: 1166816
I filed bug 1166816 to land that assertion.
You need to log in before you can comment on or make changes to this bug.