From 125450a487cbd330f63c686a55d34ecb8594710b Mon Sep 17 00:00:00 2001 From: Seth Fowler Date: Mon, 1 Aug 2016 16:44:39 -0700 Subject: [PATCH] Bug 1291033 (Part 1) - Ensure atomicity of ISurfaceProvider availability changes. r=dholbert --- image/ISurfaceProvider.h | 24 +++++++++++----- image/SurfaceCache.cpp | 58 +++++++++++++++++++++++++++++++++---- image/SurfaceCache.h | 62 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 13 deletions(-) diff --git a/image/ISurfaceProvider.h b/image/ISurfaceProvider.h index 90084be0d240..6a7f0a9affe7 100644 --- a/image/ISurfaceProvider.h +++ b/image/ISurfaceProvider.h @@ -18,6 +18,7 @@ #include "mozilla/gfx/2D.h" #include "imgFrame.h" +#include "SurfaceCache.h" namespace mozilla { namespace image { @@ -37,11 +38,6 @@ class ISurfaceProvider /// @return a drawable reference to a surface. virtual DrawableFrameRef DrawableRef() = 0; - /// @return true if this ISurfaceProvider is acting as a placeholder, which is - /// to say that it doesn't have a surface and hence can't return a - /// non-empty DrawableFrameRef yet, but it will be able to in the future. - virtual bool IsPlaceholder() const = 0; - /// @return true if DrawableRef() will return a completely decoded surface. virtual bool IsFinished() const = 0; @@ -57,8 +53,22 @@ class ISurfaceProvider /// overhead is ignored. virtual size_t LogicalSizeInBytes() const = 0; + /// @return the availability state of this ISurfaceProvider, which indicates + /// whether DrawableRef() could successfully return a surface. Should only be + /// called from SurfaceCache code as it relies on SurfaceCache for + /// synchronization. + AvailabilityState& Availability() { return mAvailability; } + const AvailabilityState& Availability() const { return mAvailability; } + protected: + explicit ISurfaceProvider(AvailabilityState aAvailability) + : mAvailability(aAvailability) + { } + virtual ~ISurfaceProvider() { } + +private: + AvailabilityState mAvailability; }; /** @@ -70,11 +80,11 @@ class SimpleSurfaceProvider final : public ISurfaceProvider NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SimpleSurfaceProvider, override) explicit SimpleSurfaceProvider(NotNull aSurface) - : mSurface(aSurface) + : ISurfaceProvider(AvailabilityState::StartAvailable()) + , mSurface(aSurface) { } DrawableFrameRef DrawableRef() override { return mSurface->DrawableRef(); } - bool IsPlaceholder() const override { return false; } bool IsFinished() const override { return mSurface->IsFinished(); } bool IsLocked() const override { return bool(mLockRef); } diff --git a/image/SurfaceCache.cpp b/image/SurfaceCache.cpp index a75ccb863b0f..49d824702646 100644 --- a/image/SurfaceCache.cpp +++ b/image/SurfaceCache.cpp @@ -71,6 +71,8 @@ typedef size_t Cost; // Placeholders do not have surfaces, but need to be given a trivial cost for // our invariants to hold. +// XXX(seth): This is only true of old-style placeholders inserted via +// InsertPlaceholder(). static const Cost sPlaceholderCost = 1; static Cost @@ -141,8 +143,8 @@ class CachedSurface , mImageKey(aImageKey) , mSurfaceKey(aSurfaceKey) { - MOZ_ASSERT(!IsPlaceholder() || mCost == sPlaceholderCost, - "Placeholder should have trivial cost"); + MOZ_ASSERT(aProvider || mCost == sPlaceholderCost, + "Old-style placeholders should have trivial cost"); MOZ_ASSERT(mImageKey, "Must have a valid image key"); } @@ -168,7 +170,11 @@ class CachedSurface mProvider->SetLocked(aLocked); } - bool IsPlaceholder() const { return !mProvider || mProvider->IsPlaceholder(); } + bool IsPlaceholder() const + { + return !mProvider || mProvider->Availability().IsPlaceholder(); + } + bool IsLocked() const { return !IsPlaceholder() && mProvider->IsLocked(); } ImageKey GetImageKey() const { return mImageKey; } @@ -449,7 +455,8 @@ class SurfaceCacheImpl final : public nsIMemoryReporter InsertOutcome Insert(ISurfaceProvider* aProvider, const Cost aCost, const ImageKey aImageKey, - const SurfaceKey& aSurfaceKey) + const SurfaceKey& aSurfaceKey, + bool aSetAvailable) { // If this is a duplicate surface, refuse to replace the original. // XXX(seth): Calling Lookup() and then RemoveEntry() does the lookup @@ -490,6 +497,11 @@ class SurfaceCacheImpl final : public nsIMemoryReporter mImageCaches.Put(aImageKey, cache); } + // If we were asked to mark the cache entry available, do so. + if (aSetAvailable) { + aProvider->Availability().SetAvailable(); + } + RefPtr surface = new CachedSurface(aProvider, aCost, aImageKey, aSurfaceKey); @@ -667,6 +679,25 @@ class SurfaceCacheImpl final : public nsIMemoryReporter return size_t(mMaxCost); } + void SurfaceAvailable(NotNull aProvider, + const ImageKey aImageKey, + const SurfaceKey& aSurfaceKey) + { + if (!aProvider->Availability().IsPlaceholder()) { + MOZ_ASSERT_UNREACHABLE("Calling SurfaceAvailable on non-placeholder"); + return; + } + + // Reinsert the provider, requesting that Insert() mark it available. This + // may or may not succeed, depending on whether some other decoder has + // beaten us to the punch and inserted a non-placeholder version of this + // surface first, but it's fine either way. + // XXX(seth): This could be implemented more efficiently; we should be able + // to just update our data structures without reinserting. + Cost cost = aProvider->LogicalSizeInBytes(); + Insert(aProvider, cost, aImageKey, aSurfaceKey, /* aSetAvailable = */ true); + } + void LockImage(const ImageKey aImageKey) { RefPtr cache = GetImageCache(aImageKey); @@ -1039,7 +1070,8 @@ SurfaceCache::Insert(NotNull aProvider, MutexAutoLock lock(sInstance->GetMutex()); Cost cost = aProvider->LogicalSizeInBytes(); - return sInstance->Insert(aProvider.get(), cost, aImageKey, aSurfaceKey); + return sInstance->Insert(aProvider.get(), cost, aImageKey, aSurfaceKey, + /* aSetAvailable = */ false); } /* static */ InsertOutcome @@ -1051,7 +1083,8 @@ SurfaceCache::InsertPlaceholder(const ImageKey aImageKey, } MutexAutoLock lock(sInstance->GetMutex()); - return sInstance->Insert(nullptr, sPlaceholderCost, aImageKey, aSurfaceKey); + return sInstance->Insert(nullptr, sPlaceholderCost, aImageKey, aSurfaceKey, + /* aSetAvailable = */ false); } /* static */ bool @@ -1075,6 +1108,19 @@ SurfaceCache::CanHold(size_t aSize) return sInstance->CanHold(aSize); } +/* static */ void +SurfaceCache::SurfaceAvailable(NotNull aProvider, + const ImageKey aImageKey, + const SurfaceKey& aSurfaceKey) +{ + if (!sInstance) { + return; + } + + MutexAutoLock lock(sInstance->GetMutex()); + sInstance->SurfaceAvailable(aProvider, aImageKey, aSurfaceKey); +} + /* static */ void SurfaceCache::LockImage(Image* aImageKey) { diff --git a/image/SurfaceCache.h b/image/SurfaceCache.h index 38055328d64b..1c6e0b6ae5dd 100644 --- a/image/SurfaceCache.h +++ b/image/SurfaceCache.h @@ -29,6 +29,7 @@ namespace image { class Image; class ISurfaceProvider; class LookupResult; +class SurfaceCacheImpl; struct SurfaceMemoryCounter; /* @@ -118,6 +119,36 @@ VectorSurfaceKey(const gfx::IntSize& aSize, return SurfaceKey(aSize, aSVGContext, aAnimationTime, DefaultSurfaceFlags()); } + +/** + * AvailabilityState is used to track whether an ISurfaceProvider has a surface + * available or is just a placeholder. + * + * To ensure that availability changes are atomic (and especially that internal + * SurfaceCache code doesn't have to deal with asynchronous availability + * changes), an ISurfaceProvider which starts as a placeholder can only reveal + * the fact that it now has a surface available via a call to + * SurfaceCache::SurfaceAvailable(). + */ +class AvailabilityState +{ +public: + static AvailabilityState StartAvailable() { return AvailabilityState(true); } + static AvailabilityState StartAsPlaceholder() { return AvailabilityState(false); } + + bool IsAvailable() const { return mIsAvailable; } + bool IsPlaceholder() const { return !mIsAvailable; } + +private: + friend class SurfaceCacheImpl; + + explicit AvailabilityState(bool aIsAvailable) : mIsAvailable(aIsAvailable) { } + + void SetAvailable() { mIsAvailable = true; } + + bool mIsAvailable; +}; + enum class InsertOutcome : uint8_t { SUCCESS, // Success (but see Insert documentation). FAILURE, // Couldn't insert (e.g., for capacity reasons). @@ -283,6 +314,37 @@ struct SurfaceCache static InsertOutcome InsertPlaceholder(const ImageKey aImageKey, const SurfaceKey& aSurfaceKey); + /** + * Mark the cache entry @aProvider as having an available surface. This turns + * a placeholder cache entry into a normal cache entry. The cache entry + * becomes locked if the associated image is locked; otherwise, it starts in + * the unlocked state. + * + * If the cache entry containing @aProvider has already been evicted from the + * surface cache, this function has no effect. + * + * It's illegal to call this function if @aProvider is not a placeholder; by + * definition, non-placeholder ISurfaceProviders should have a surface + * available already. + * + * XXX(seth): We're currently in a transitional state where two notions of + * placeholder exist: the old one (placeholders are an "empty" cache entry + * inserted via InsertPlaceholder(), which then gets replaced by inserting a + * real cache entry with the same keys via Insert()) and the new one (where + * the same cache entry, inserted via Insert(), starts in a placeholder state + * and then transitions to being a normal cache entry via this function). The + * old mechanism will be removed in bug 1292392. + * + * @param aProvider The cache entry that now has a surface available. + * @param aImageKey Key data identifying which image the cache entry + * belongs to. + * @param aSurfaceKey Key data which uniquely identifies the requested + * cache entry. + */ + static void SurfaceAvailable(NotNull aProvider, + const ImageKey aImageKey, + const SurfaceKey& aSurfaceKey); + /** * Checks if a surface of a given size could possibly be stored in the cache. * If CanHold() returns false, Insert() will always fail to insert the