From aaf43f6f994e1d6c417f1026ffa6eaf6e9d901f7 Mon Sep 17 00:00:00 2001
From: Seth Fowler <seth@mozilla.com>
Date: Sat, 31 Jan 2015 15:29:48 -0800
Subject: [PATCH] Bug 1126739 - Add locking to SurfaceCache entry points that
 bypass the public API. r=tn

---
 image/src/RasterImage.cpp               | 13 +++------
 image/src/SurfaceCache.cpp              | 16 +++++------
 image/test/browser/browser_bug666317.js | 38 ++++++++++++++++---------
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/image/src/RasterImage.cpp b/image/src/RasterImage.cpp
index 7d44a22829a..b805bb524f4 100644
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -614,16 +614,11 @@ RasterImage::IsOpaque()
 void
 RasterImage::OnSurfaceDiscarded()
 {
-  if (!NS_IsMainThread()) {
-    nsCOMPtr<nsIRunnable> runnable =
-      NS_NewRunnableMethod(this, &RasterImage::OnSurfaceDiscarded);
-    NS_DispatchToMainThread(runnable);
-    return;
-  }
+  MOZ_ASSERT(mProgressTracker);
 
-  if (mProgressTracker) {
-    mProgressTracker->OnDiscard();
-  }
+  nsCOMPtr<nsIRunnable> runnable =
+    NS_NewRunnableMethod(mProgressTracker, &ProgressTracker::OnDiscard);
+  NS_DispatchToMainThread(runnable);
 }
 
 //******************************************************************************
diff --git a/image/src/SurfaceCache.cpp b/image/src/SurfaceCache.cpp
index 54ed47404d3..10925f68d12 100644
--- a/image/src/SurfaceCache.cpp
+++ b/image/src/SurfaceCache.cpp
@@ -362,7 +362,7 @@ public:
   SurfaceCacheImpl(uint32_t aSurfaceCacheExpirationTimeMS,
                    uint32_t aSurfaceCacheDiscardFactor,
                    uint32_t aSurfaceCacheSize)
-    : mExpirationTracker(this, aSurfaceCacheExpirationTimeMS)
+    : mExpirationTracker(aSurfaceCacheExpirationTimeMS)
     , mMemoryPressureObserver(new MemoryPressureObserver)
     , mMutex("SurfaceCache")
     , mDiscardFactor(aSurfaceCacheDiscardFactor)
@@ -742,6 +742,8 @@ public:
                  nsISupports*             aData,
                  bool                     aAnonymize) MOZ_OVERRIDE
   {
+    MutexAutoLock lock(mMutex);
+
     // We have explicit memory reporting for the surface cache which is more
     // accurate than the cost metrics we report here, but these metrics are
     // still useful to report, since they control the cache's behavior.
@@ -809,21 +811,18 @@ private:
 
   struct SurfaceTracker : public nsExpirationTracker<CachedSurface, 2>
   {
-    SurfaceTracker(SurfaceCacheImpl* aCache, uint32_t aSurfaceCacheExpirationTimeMS)
+    explicit SurfaceTracker(uint32_t aSurfaceCacheExpirationTimeMS)
       : nsExpirationTracker<CachedSurface, 2>(aSurfaceCacheExpirationTimeMS)
-      , mCache(aCache)
     { }
 
   protected:
     virtual void NotifyExpired(CachedSurface* aSurface) MOZ_OVERRIDE
     {
-      if (mCache) {
-        mCache->Remove(aSurface);
+      if (sInstance) {
+        MutexAutoLock lock(sInstance->GetMutex());
+        sInstance->Remove(aSurface);
       }
     }
-
-  private:
-    SurfaceCacheImpl* const mCache;  // Weak pointer to owner.
   };
 
   struct MemoryPressureObserver : public nsIObserver
@@ -835,6 +834,7 @@ private:
                        const char16_t*) MOZ_OVERRIDE
     {
       if (sInstance && strcmp(aTopic, "memory-pressure") == 0) {
+        MutexAutoLock lock(sInstance->GetMutex());
         sInstance->DiscardForMemoryPressure();
       }
       return NS_OK;
diff --git a/image/test/browser/browser_bug666317.js b/image/test/browser/browser_bug666317.js
index 15064760890..ed7024db399 100644
--- a/image/test/browser/browser_bug666317.js
+++ b/image/test/browser/browser_bug666317.js
@@ -27,18 +27,6 @@ function currentRequest() {
   return img.getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST);
 }
 
-function attachDiscardObserver(result) {
-  // Create the discard observer.
-  let observer = new ImageDiscardObserver(result);
-  let scriptedObserver = Cc["@mozilla.org/image/tools;1"]
-                           .getService(Ci.imgITools)
-                           .createScriptedObserver(observer);
-
-  // Clone the current imgIRequest with our new observer.
-  let request = currentRequest();
-  return request.clone(scriptedObserver);
-}
-
 function isImgDecoded() {
   let request = currentRequest();
   return request.imageStatus & Ci.imgIRequest.STATUS_FRAME_COMPLETE ? true : false;
@@ -69,9 +57,18 @@ function test() {
 }
 
 function step2() {
-  // Attach a discard listener and create a place to hold the result.
+  // Create a place to hold the result.
   var result = { wasDiscarded: false };
-  var clonedRequest = attachDiscardObserver(result);
+
+  // Create the discard observer.
+  var observer = new ImageDiscardObserver(result);
+  var scriptedObserver = Cc["@mozilla.org/image/tools;1"]
+                           .getService(Ci.imgITools)
+                           .createScriptedObserver(observer);
+
+  // Clone the current imgIRequest with our new observer.
+  var request = currentRequest();
+  var clonedRequest = request.clone(scriptedObserver);
 
   // Check that the image is decoded.
   forceDecodeImg();
@@ -83,6 +80,19 @@ function step2() {
   var os = Cc["@mozilla.org/observer-service;1"]
              .getService(Ci.nsIObserverService);
   os.notifyObservers(null, 'memory-pressure', 'heap-minimize');
+
+  // The discard notification is delivered asynchronously, so pump the event
+  // loop before checking.
+  window.addEventListener('message', function (event) {
+    if (event.data == 'step3') {
+      step3(result, scriptedObserver, clonedRequest);
+    }
+  }, false);
+
+  window.postMessage('step3', '*');
+}
+
+function step3(result, scriptedObserver, clonedRequest) {
   ok(result.wasDiscarded, 'Image should be discarded.');
 
   // And we're done.