Bug 1388332 - Fix a shutdown crash when destroying image decoders before initializing its SurfacePipe. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 09 Aug 2017 06:54:55 -0400
changeset 373584 125b73295e1e6ada26a50ce8882fc27847ff2eb5
parent 373583 704c58d8803e7a0388111af34325dc1964cd4a3b
child 373585 0dc9284b03d41e8ed9096915fd95e18c8b99951c
push id32305
push userryanvm@gmail.com
push dateWed, 09 Aug 2017 22:43:53 +0000
treeherdermozilla-central@411fe4772f31 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1388332
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1388332 - Fix a shutdown crash when destroying image decoders before initializing its SurfacePipe. r=tnikkel A default constructed SurfacePipe contains a NullSurfaceSink as its filter in mHead. This filter does nothing and is merely a placeholder. Since most SurfacePipe objects are constructed with the default constructor, and NullSurfaceSink has no (modified) state, we use a singleton to represent it. Normally the SurfacePipe owns its filter, so it needs to do a special check for NullSurfaceSink to ensure it doesn't free it explicitly. A Decoder object contains a default constructed SurfacePipe until it needs to create the first frame from an image. This is a very brief window because it does not take very long or much data to get to this stage of decoding. The NullSurfaceSink singleton is freed upon shutdown, however some ISurfaceProvider objects may be lingering after this. If their Decoder has yet to create the first frame, that means the SurfacePipe actually contains a dangling pointer to the already freed singleton. To make things worse, it actually tried to free the filter because it didn't match the singleton (it got freed!). As such, this change removes NullSurfaceSink entirely. We never use the SurfacePipe before initializing it with a proper filter, and it would be considered a programming error to do so. Instead let SurfacePipe::mHead be null, and assert that it is not null when any operations are performed on the SurfacePipe.
image/SurfacePipe.cpp
image/SurfacePipe.h
image/build/nsImageModule.cpp
image/test/gtest/TestSurfaceSink.cpp
--- a/image/SurfacePipe.cpp
+++ b/image/SurfacePipe.cpp
@@ -1,56 +1,27 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "SurfacePipe.h"
 
-#include <utility>
+#include <algorithm>    // for min
 
-#include "mozilla/ClearOnShutdown.h"
-#include "mozilla/DebugOnly.h"
 #include "Decoder.h"
 
 namespace mozilla {
 namespace image {
 
 using namespace gfx;
 
 using std::min;
 
-/* static */ UniquePtr<NullSurfaceSink> NullSurfaceSink::sSingleton;
-
-/* static */ NullSurfaceSink*
-NullSurfaceSink::Singleton()
-{
-  if (!sSingleton) {
-    MOZ_ASSERT(NS_IsMainThread());
-    sSingleton = MakeUnique<NullSurfaceSink>();
-    ClearOnShutdown(&sSingleton);
-
-    DebugOnly<nsresult> rv = sSingleton->Configure(NullSurfaceConfig { });
-    MOZ_ASSERT(NS_SUCCEEDED(rv), "Couldn't configure a NullSurfaceSink?");
-  }
-
-  return sSingleton.get();
-}
-
-nsresult
-NullSurfaceSink::Configure(const NullSurfaceConfig& aConfig)
-{
-  // Note that the choice of uint32_t as the pixel size here is more or less
-  // arbitrary, since you cannot write to a NullSurfaceSink anyway, but uint32_t
-  // is a natural choice since most SurfacePipes will be for BGRA/BGRX surfaces.
-  ConfigureFilter(IntSize(), sizeof(uint32_t));
-  return NS_OK;
-}
-
 Maybe<SurfaceInvalidRect>
 AbstractSurfaceSink::TakeInvalidRect()
 {
   if (mInvalidRect.IsEmpty()) {
     return Nothing();
   }
 
   SurfaceInvalidRect invalidRect;
--- a/image/SurfacePipe.h
+++ b/image/SurfacePipe.h
@@ -509,133 +509,87 @@ private:
   }
 
   gfx::IntSize mInputSize;  /// The size of the input this filter expects.
   uint8_t* mRowPointer;     /// Pointer to the current row or null if finished.
   int32_t mCol;             /// The current column we're writing to. (0-indexed)
   uint8_t  mPixelSize;      /// How large each pixel in the surface is, in bytes.
 };
 
-class NullSurfaceSink;
-
-/// A trivial configuration struct for NullSurfaceSink.
-struct NullSurfaceConfig
-{
-  using Filter = NullSurfaceSink;
-};
-
-/**
- * NullSurfaceSink is a trivial SurfaceFilter implementation that behaves as if
- * it were a zero-size SurfaceSink. It's used as the default filter chain for an
- * uninitialized SurfacePipe.
- *
- * To avoid unnecessary allocations when creating SurfacePipe objects,
- * NullSurfaceSink is a singleton. (This implies that the implementation must be
- * stateless.)
- */
-class NullSurfaceSink final : public SurfaceFilter
-{
-public:
-  /// Returns the singleton instance of NullSurfaceSink.
-  static NullSurfaceSink* Singleton();
-
-  virtual ~NullSurfaceSink() { }
-
-  nsresult Configure(const NullSurfaceConfig& aConfig);
-
-  Maybe<SurfaceInvalidRect> TakeInvalidRect() override { return Nothing(); }
-
-protected:
-  uint8_t* DoResetToFirstRow() override { return nullptr; }
-  uint8_t* DoAdvanceRow() override { return nullptr; }
-
-private:
-  static UniquePtr<NullSurfaceSink> sSingleton;  /// The singleton instance.
-};
-
-
 /**
  * SurfacePipe is the public API that decoders should use to interact with a
  * SurfaceFilter pipeline.
  */
 class SurfacePipe
 {
 public:
-  /// Initialize global state used by all SurfacePipes.
-  static void Initialize() { NullSurfaceSink::Singleton(); }
-
   SurfacePipe()
-    : mHead(NullSurfaceSink::Singleton())
   { }
 
   SurfacePipe(SurfacePipe&& aOther)
     : mHead(Move(aOther.mHead))
   { }
 
   ~SurfacePipe()
-  {
-    // Ensure that we don't free the NullSurfaceSink singleton.
-    if (mHead.get() == NullSurfaceSink::Singleton()) {
-      Unused << mHead.release();
-    }
-  }
+  { }
 
   SurfacePipe& operator=(SurfacePipe&& aOther)
   {
     MOZ_ASSERT(this != &aOther);
-
-    // Ensure that we don't free the NullSurfaceSink singleton.
-    if (mHead.get() == NullSurfaceSink::Singleton()) {
-      Unused << mHead.release();
-    }
-
     mHead = Move(aOther.mHead);
     return *this;
   }
 
   /// Begins a new pass, seeking to the first row of the surface.
-  void ResetToFirstRow() { mHead->ResetToFirstRow(); }
+  void ResetToFirstRow()
+  {
+    MOZ_ASSERT(mHead, "Use before configured!");
+    mHead->ResetToFirstRow();
+  }
 
   /**
    * Write pixels to the surface one at a time by repeatedly calling a lambda
    * that yields pixels. WritePixels() is completely memory safe.
    *
    * @see SurfaceFilter::WritePixels() for the canonical documentation.
    */
   template <typename PixelType, typename Func>
   WriteState WritePixels(Func aFunc)
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->WritePixels<PixelType>(Forward<Func>(aFunc));
   }
 
   /**
    * A variant of WritePixels() that writes a single row of pixels to the
    * surface one at a time by repeatedly calling a lambda that yields pixels.
    * WritePixelsToRow() is completely memory safe.
    *
    * @see SurfaceFilter::WritePixelsToRow() for the canonical documentation.
    */
   template <typename PixelType, typename Func>
   WriteState WritePixelsToRow(Func aFunc)
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->WritePixelsToRow<PixelType>(Forward<Func>(aFunc));
   }
 
   /**
    * Write a row to the surface by copying from a buffer. This is bounds checked
    * and memory safe with respect to the surface, but care must still be taken
    * by the caller not to overread the source buffer. This variant of
    * WriteBuffer() requires a source buffer which contains |mInputSize.width|
    * pixels.
    *
    * @see SurfaceFilter::WriteBuffer() for the canonical documentation.
    */
   template <typename PixelType>
   WriteState WriteBuffer(const PixelType* aSource)
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->WriteBuffer<PixelType>(aSource);
   }
 
   /**
    * Write a row to the surface by copying from a buffer. This is bounds checked
    * and memory safe with respect to the surface, but care must still be taken
    * by the caller not to overread the source buffer. This variant of
    * WriteBuffer() reads at most @aLength pixels from the buffer and writes them
@@ -644,36 +598,39 @@ public:
    *
    * @see SurfaceFilter::WriteBuffer() for the canonical documentation.
    */
   template <typename PixelType>
   WriteState WriteBuffer(const PixelType* aSource,
                          const size_t aStartColumn,
                          const size_t aLength)
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->WriteBuffer<PixelType>(aSource, aStartColumn, aLength);
   }
 
   /**
    * Write an empty row to the surface. If some pixels have already been written
    * to this row, they'll be discarded.
    *
    * @see SurfaceFilter::WriteEmptyRow() for the canonical documentation.
    */
   WriteState WriteEmptyRow()
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->WriteEmptyRow();
   }
 
   /// @return true if we've finished writing to the surface.
   bool IsSurfaceFinished() const { return mHead->IsSurfaceFinished(); }
 
   /// @see SurfaceFilter::TakeInvalidRect() for the canonical documentation.
   Maybe<SurfaceInvalidRect> TakeInvalidRect() const
   {
+    MOZ_ASSERT(mHead, "Use before configured!");
     return mHead->TakeInvalidRect();
   }
 
 private:
   friend class SurfacePipeFactory;
   friend class TestSurfacePipeFactory;
 
   explicit SurfacePipe(UniquePtr<SurfaceFilter>&& aHead)
--- a/image/build/nsImageModule.cpp
+++ b/image/build/nsImageModule.cpp
@@ -98,17 +98,16 @@ mozilla::image::EnsureModuleInitialized(
 
   // Make sure the preferences are initialized
   gfxPrefs::GetSingleton();
 
   mozilla::image::ShutdownTracker::Initialize();
   mozilla::image::ImageFactory::Initialize();
   mozilla::image::DecodePool::Initialize();
   mozilla::image::SurfaceCache::Initialize();
-  mozilla::image::SurfacePipe::Initialize();
   imgLoader::GlobalInit();
   sInitialized = true;
   return NS_OK;
 }
 
 void
 mozilla::image::ShutdownModule()
 {
--- a/image/test/gtest/TestSurfaceSink.cpp
+++ b/image/test/gtest/TestSurfaceSink.cpp
@@ -108,81 +108,16 @@ CheckPalettedIterativeWrite(Decoder* aDe
     return aWriteFunc();
   };
 
   DoCheckIterativeWrite(aSink, writeFunc, [&]{
     CheckGeneratedPalettedImage(aDecoder, aOutputRect);
   });
 }
 
-TEST(ImageSurfaceSink, NullSurfaceSink)
-{
-  // Create the NullSurfaceSink.
-  NullSurfaceSink sink;
-  nsresult rv = sink.Configure(NullSurfaceConfig { });
-  ASSERT_TRUE(NS_SUCCEEDED(rv));
-  EXPECT_TRUE(!sink.IsValidPalettedPipe());
-
-  // Ensure that we can't write anything.
-  bool gotCalled = false;
-  auto result = sink.WritePixels<uint32_t>([&]() {
-    gotCalled = true;
-    return AsVariant(BGRAColor::Green().AsPixel());
-  });
-  EXPECT_FALSE(gotCalled);
-  EXPECT_EQ(WriteState::FINISHED, result);
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  Maybe<SurfaceInvalidRect> invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  uint32_t source = BGRAColor::Red().AsPixel();
-  result = sink.WriteBuffer(&source);
-  EXPECT_EQ(WriteState::FINISHED, result);
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  result = sink.WriteBuffer(&source, 0, 1);
-  EXPECT_EQ(WriteState::FINISHED, result);
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  result = sink.WriteEmptyRow();
-  EXPECT_EQ(WriteState::FINISHED, result);
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  result = sink.WriteUnsafeComputedRow<uint32_t>([&](uint32_t* aRow,
-                                                     uint32_t aLength) {
-    gotCalled = true;
-    for (uint32_t col = 0; col < aLength; ++col, ++aRow) {
-      *aRow = BGRAColor::Red().AsPixel();
-    }
-  });
-  EXPECT_FALSE(gotCalled);
-  EXPECT_EQ(WriteState::FINISHED, result);
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  // Attempt to advance to the next row and make sure nothing changes.
-  sink.AdvanceRow();
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-
-  // Attempt to advance to the next pass and make sure nothing changes.
-  sink.ResetToFirstRow();
-  EXPECT_TRUE(sink.IsSurfaceFinished());
-  invalidRect = sink.TakeInvalidRect();
-  EXPECT_TRUE(invalidRect.isNothing());
-}
-
 TEST(ImageSurfaceSink, SurfaceSinkInitialization)
 {
   WithSurfaceSink<Orient::NORMAL>([](Decoder* aDecoder, SurfaceSink* aSink) {
     // Check initial state.
     EXPECT_FALSE(aSink->IsSurfaceFinished());
     Maybe<SurfaceInvalidRect> invalidRect = aSink->TakeInvalidRect();
     EXPECT_TRUE(invalidRect.isNothing());