Backed out changeset 45dd83a63162 (bug 1339202) for crashing in image processing related tests, e.g. xpcshell test test_imgtools.js. r=backout
authorSebastian Hengst <archaeopteryx@coole-files.de>
Mon, 13 Feb 2017 22:34:54 +0100
changeset 389313 56f67dd567514cbf30de508487daa97d44b6c2ac
parent 389312 5f74cfa4af59296179594ef6269fc35e6f2833d3
child 389314 f3dcd0723d358412c1b5e14e7b20319a10966475
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout
bugs1339202
milestone54.0a1
backs out45dd83a63162363fe69af8e65e8a79b24827ce38
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
Backed out changeset 45dd83a63162 (bug 1339202) for crashing in image processing related tests, e.g. xpcshell test test_imgtools.js. r=backout
image/Decoder.cpp
image/Decoder.h
image/DecoderFactory.cpp
image/RasterImage.cpp
image/decoders/nsICODecoder.cpp
image/imgFrame.cpp
image/imgFrame.h
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -59,17 +59,16 @@ Decoder::Decoder(RasterImage* aImage)
   , mMetadataDecode(false)
   , mHaveExplicitOutputSize(false)
   , mInFrame(false)
   , mFinishedNewFrame(false)
   , mReachedTerminalState(false)
   , mDecodeDone(false)
   , mError(false)
   , mShouldReportError(false)
-  , mFinalizeFrames(true)
 { }
 
 Decoder::~Decoder()
 {
   MOZ_ASSERT(mProgress == NoProgress || !mImage,
              "Destroying Decoder without taking all its progress changes");
   MOZ_ASSERT(mInvalidRect.IsEmpty() || !mImage,
              "Destroying Decoder without taking all its invalidations");
@@ -451,17 +450,17 @@ Decoder::PostFrameStop(Opacity aFrameOpa
   MOZ_ASSERT(mInFrame, "Stopping frame when we didn't start one");
   MOZ_ASSERT(mCurrentFrame, "Stopping frame when we don't have one");
 
   // Update our state.
   mInFrame = false;
   mFinishedNewFrame = true;
 
   mCurrentFrame->Finish(aFrameOpacity, aDisposalMethod, aTimeout,
-                        aBlendMethod, aBlendRect, mFinalizeFrames);
+                        aBlendMethod, aBlendRect);
 
   mProgress |= FLAG_FRAME_COMPLETE;
 
   mLoopLength += aTimeout;
 
   // If we're not sending partial invalidations, then we send an invalidation
   // here when the first frame is complete.
   if (!ShouldSendPartialInvalidations() && mFrameCount == 1) {
--- a/image/Decoder.h
+++ b/image/Decoder.h
@@ -257,20 +257,16 @@ public:
 
   // Did we discover that the image we're decoding is animated?
   bool HasAnimation() const { return mImageMetadata.HasAnimation(); }
 
   // Error tracking
   bool HasError() const { return mError; }
   bool ShouldReportError() const { return mShouldReportError; }
 
-  // Finalize frames
-  void SetFinalizeFrames(bool aFinalize) { mFinalizeFrames = aFinalize; }
-  bool GetFinalizeFrames() const { return mFinalizeFrames; }
-
   /// Did we finish decoding enough that calling Decode() again would be useless?
   bool GetDecodeDone() const
   {
     return mReachedTerminalState || mDecodeDone ||
            (mMetadataDecode && HasSize()) || HasError();
   }
 
   /// Are we in the middle of a frame right now? Used for assertions only.
@@ -545,15 +541,14 @@ private:
   bool mHaveExplicitOutputSize : 1;
   bool mInFrame : 1;
   bool mFinishedNewFrame : 1;  // True if PostFrameStop() has been called since
                                // the last call to TakeCompleteFrameCount().
   bool mReachedTerminalState : 1;
   bool mDecodeDone : 1;
   bool mError : 1;
   bool mShouldReportError : 1;
-  bool mFinalizeFrames : 1;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_Decoder_h
--- a/image/DecoderFactory.cpp
+++ b/image/DecoderFactory.cpp
@@ -261,17 +261,16 @@ DecoderFactory::CreateDecoderForICOResou
 
   // Initialize the decoder, copying settings from @aICODecoder.
   MOZ_ASSERT(!aICODecoder->IsMetadataDecode());
   decoder->SetMetadataDecode(aICODecoder->IsMetadataDecode());
   decoder->SetIterator(aSourceBuffer->Iterator());
   decoder->SetOutputSize(aICODecoder->OutputSize());
   decoder->SetDecoderFlags(aICODecoder->GetDecoderFlags());
   decoder->SetSurfaceFlags(aICODecoder->GetSurfaceFlags());
-  decoder->SetFinalizeFrames(false);
 
   if (NS_FAILED(decoder->Init())) {
     return nullptr;
   }
 
   return decoder.forget();
 }
 
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -474,30 +474,25 @@ RasterImage::GetFirstFrameDelay()
   MOZ_ASSERT(mAnimationState, "Animated images should have an AnimationState");
   return mAnimationState->FirstFrameTimeout().AsEncodedValueDeprecated();
 }
 
 NS_IMETHODIMP_(already_AddRefed<SourceSurface>)
 RasterImage::GetFrame(uint32_t aWhichFrame,
                       uint32_t aFlags)
 {
-  return GetFrameAtSize(mSize, aWhichFrame, aFlags);
+  return GetFrameInternal(mSize, aWhichFrame, aFlags).second().forget();
 }
 
 NS_IMETHODIMP_(already_AddRefed<SourceSurface>)
 RasterImage::GetFrameAtSize(const IntSize& aSize,
                             uint32_t aWhichFrame,
                             uint32_t aFlags)
 {
-  RefPtr<SourceSurface> surf =
-    GetFrameInternal(aSize, aWhichFrame, aFlags).second().forget();
-  // If we are here, it suggests the image is embedded in a canvas or some
-  // other path besides layers, and we won't need the file handle.
-  MarkSurfaceShared(surf);
-  return surf.forget();
+  return GetFrameInternal(aSize, aWhichFrame, aFlags).second().forget();
 }
 
 Pair<DrawResult, RefPtr<SourceSurface>>
 RasterImage::GetFrameInternal(const IntSize& aSize,
                               uint32_t aWhichFrame,
                               uint32_t aFlags)
 {
   MOZ_ASSERT(aWhichFrame <= FRAME_MAX_VALUE);
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -588,23 +588,16 @@ nsICODecoder::FinishResource()
 {
   // Make sure the actual size of the resource matches the size in the directory
   // entry. If not, we consider the image corrupt.
   if (mContainedDecoder->HasSize() &&
       mContainedDecoder->Size() != GetRealSize()) {
     return Transition::TerminateFailure();
   }
 
-  // Finalize the frame which we deferred to ensure we could modify the final
-  // result (e.g. to apply the BMP mask).
-  MOZ_ASSERT(!mContainedDecoder->GetFinalizeFrames());
-  if (mCurrentFrame) {
-    mCurrentFrame->FinalizeSurface();
-  }
-
   return Transition::TerminateSuccess();
 }
 
 LexerResult
 nsICODecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
 {
   MOZ_ASSERT(!HasError(), "Shouldn't call DoDecode after error!");
 
--- a/image/imgFrame.cpp
+++ b/image/imgFrame.cpp
@@ -7,25 +7,24 @@
 #include "imgFrame.h"
 #include "ImageRegion.h"
 #include "ShutdownTracker.h"
 
 #include "prenv.h"
 
 #include "gfx2DGlue.h"
 #include "gfxPlatform.h"
+#include "gfxPrefs.h"
 #include "gfxUtils.h"
 #include "gfxAlphaRecovery.h"
 
 #include "GeckoProfiler.h"
 #include "MainThreadUtils.h"
 #include "mozilla/CheckedInt.h"
 #include "mozilla/gfx/Tools.h"
-#include "mozilla/gfx/gfxVars.h"
-#include "mozilla/layers/SourceSurfaceSharedData.h"
 #include "mozilla/layers/SourceSurfaceVolatileData.h"
 #include "mozilla/Likely.h"
 #include "mozilla/MemoryReporting.h"
 #include "nsMargin.h"
 #include "nsThreadUtils.h"
 
 
 namespace mozilla {
@@ -47,22 +46,16 @@ VolatileSurfaceStride(const IntSize& siz
   return (size.width * BytesPerPixel(format) + 0x3) & ~0x3;
 }
 
 static already_AddRefed<DataSourceSurface>
 CreateLockedSurface(DataSourceSurface *aSurface,
                     const IntSize& size,
                     SurfaceFormat format)
 {
-  // Shared memory is never released until the surface itself is released
-  if (aSurface->GetType() == SurfaceType::DATA_SHARED) {
-    RefPtr<DataSourceSurface> surf(aSurface);
-    return surf.forget();
-  }
-
   DataSourceSurface::ScopedMap* smap =
     new DataSourceSurface::ScopedMap(aSurface, DataSourceSurface::READ_WRITE);
   if (smap->IsMapped()) {
     // The ScopedMap is held by this DataSourceSurface.
     RefPtr<DataSourceSurface> surf =
       Factory::CreateWrappingDataSourceSurface(smap->GetData(),
                                                aSurface->Stride(),
                                                size,
@@ -79,27 +72,21 @@ CreateLockedSurface(DataSourceSurface *a
 }
 
 static already_AddRefed<DataSourceSurface>
 AllocateBufferForImage(const IntSize& size,
                        SurfaceFormat format,
                        bool aIsAnimated = false)
 {
   int32_t stride = VolatileSurfaceStride(size, format);
-  if (!aIsAnimated && gfxVars::UseWebRender()) {
-    RefPtr<SourceSurfaceSharedData> newSurf = new SourceSurfaceSharedData();
-    if (newSurf->Init(size, stride, format)) {
-      return newSurf.forget();
-    }
-  } else {
-    RefPtr<SourceSurfaceVolatileData> newSurf= new SourceSurfaceVolatileData();
-    if (newSurf->Init(size, stride, format)) {
-      return newSurf.forget();
-    }
+  RefPtr<SourceSurfaceVolatileData> newSurf = new SourceSurfaceVolatileData();
+  if (newSurf->Init(size, stride, format)) {
+    return newSurf.forget();
   }
+
   return nullptr;
 }
 
 static bool
 ClearSurface(DataSourceSurface* aSurface, const IntSize& aSize, SurfaceFormat aFormat)
 {
   int32_t stride = aSurface->Stride();
   uint8_t* data = aSurface->GetData();
@@ -116,29 +103,16 @@ ClearSurface(DataSourceSurface* aSurface
     // Otherwise, it's allocated via mmap and refers to a zeroed page and will
     // be COW once it's written to.
     memset(data, 0, stride * aSize.height);
   }
 
   return true;
 }
 
-void
-MarkSurfaceShared(SourceSurface* aSurface)
-{
-  // Depending on what requested the image decoding, the buffer may or may not
-  // end up being shared with another process (e.g. put in a painted layer,
-  // used inside a canvas). If not shared, we should ensure are not keeping the
-  // handle only because we have yet to share it.
-  if (aSurface && aSurface->GetType() == SurfaceType::DATA_SHARED) {
-    auto sharedSurface = static_cast<SourceSurfaceSharedData*>(aSurface);
-    sharedSurface->FinishedSharing();
-  }
-}
-
 // Returns true if an image of aWidth x aHeight is allowed and legal.
 static bool
 AllowedImageSize(int32_t aWidth, int32_t aHeight)
 {
   // reject over-wide or over-tall images
   const int32_t k64KLimit = 0x0000FFFF;
   if (MOZ_UNLIKELY(aWidth > k64KLimit || aHeight > k64KLimit )) {
     NS_WARNING("image too big");
@@ -380,18 +354,16 @@ imgFrame::InitWithDrawable(gfxDrawable* 
     mAborted = true;
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   if (!canUseDataSurface) {
     // We used an offscreen surface, which is an "optimized" surface from
     // imgFrame's perspective.
     mOptSurface = target->Snapshot();
-  } else {
-    FinalizeSurface();
   }
 
   // If we reach this point, we should regard ourselves as complete.
   mDecoded = GetRect();
   mFinished = true;
 
 #ifdef DEBUG
   MonitorAutoLock lock(mMonitor);
@@ -574,20 +546,16 @@ bool imgFrame::Draw(gfxContext* aContext
   SurfaceWithFormat surfaceResult =
     SurfaceForDrawing(doPartialDecode, doTile, region, surf);
 
   if (surfaceResult.IsValid()) {
     gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mDrawable,
                                imageRect.Size(), region, surfaceResult.mFormat,
                                aSamplingFilter, aImageFlags, aOpacity);
   }
-
-  // Image got put into a painted layer, it will not be shared with another
-  // process.
-  MarkSurfaceShared(surf);
   return true;
 }
 
 nsresult
 imgFrame::ImageUpdated(const nsIntRect& aUpdateRect)
 {
   MonitorAutoLock lock(mMonitor);
   return ImageUpdatedInternal(aUpdateRect);
@@ -608,32 +576,26 @@ imgFrame::ImageUpdatedInternal(const nsI
 }
 
 void
 imgFrame::Finish(Opacity aFrameOpacity /* = Opacity::SOME_TRANSPARENCY */,
                  DisposalMethod aDisposalMethod /* = DisposalMethod::KEEP */,
                  FrameTimeout aTimeout
                    /* = FrameTimeout::FromRawMilliseconds(0) */,
                  BlendMethod aBlendMethod /* = BlendMethod::OVER */,
-                 const Maybe<IntRect>& aBlendRect /* = Nothing() */,
-                 bool aFinalize /* = true */)
+                 const Maybe<IntRect>& aBlendRect /* = Nothing() */)
 {
   MonitorAutoLock lock(mMonitor);
   MOZ_ASSERT(mLockCount > 0, "Image data should be locked");
 
   mDisposalMethod = aDisposalMethod;
   mTimeout = aTimeout;
   mBlendMethod = aBlendMethod;
   mBlendRect = aBlendRect;
   ImageUpdatedInternal(GetRect());
-
-  if (aFinalize) {
-    FinalizeSurfaceInternal();
-  }
-
   mFinished = true;
 
   // The image is now complete, wake up anyone who's waiting.
   mMonitor.NotifyAll();
 }
 
 uint32_t
 imgFrame::GetImageBytesPerRow() const
@@ -666,19 +628,16 @@ imgFrame::GetImageData(uint8_t** aData, 
 
 void
 imgFrame::GetImageDataInternal(uint8_t** aData, uint32_t* aLength) const
 {
   mMonitor.AssertCurrentThreadOwns();
   MOZ_ASSERT(mLockCount > 0, "Image data should be locked");
 
   if (mLockedSurface) {
-    // TODO: This is okay for now because we only realloc shared surfaces on
-    // the main thread after decoding has finished, but if animations want to
-    // read frame data off the main thread, we will need to reconsider this.
     *aData = mLockedSurface->GetData();
     MOZ_ASSERT(*aData,
       "mLockedSurface is non-null, but GetData is null in GetImageData");
   } else if (mPalettedImageData) {
     *aData = mPalettedImageData + PaletteDataLength();
     MOZ_ASSERT(*aData,
       "mPalettedImageData is non-null, but result is null in GetImageData");
   } else {
@@ -789,37 +748,16 @@ imgFrame::UnlockImageData()
 void
 imgFrame::SetOptimizable()
 {
   AssertImageDataLocked();
   MonitorAutoLock lock(mMonitor);
   mOptimizable = true;
 }
 
-void
-imgFrame::FinalizeSurface()
-{
-  MonitorAutoLock lock(mMonitor);
-  FinalizeSurfaceInternal();
-}
-
-void
-imgFrame::FinalizeSurfaceInternal()
-{
-  mMonitor.AssertCurrentThreadOwns();
-
-  // Not all images will have mRawSurface to finalize (i.e. paletted images).
-  if (!mRawSurface || mRawSurface->GetType() != SurfaceType::DATA_SHARED) {
-    return;
-  }
-
-  auto sharedSurf = static_cast<SourceSurfaceSharedData*>(mRawSurface.get());
-  sharedSurf->Finalize();
-}
-
 already_AddRefed<SourceSurface>
 imgFrame::GetSourceSurface()
 {
   MonitorAutoLock lock(mMonitor);
   return GetSourceSurfaceInternal();
 }
 
 already_AddRefed<SourceSurface>
--- a/image/imgFrame.h
+++ b/image/imgFrame.h
@@ -274,25 +274,22 @@ public:
    *                         displayed.
    * @param aTimeout         For animation frames, the timeout before the next
    *                         frame is displayed.
    * @param aBlendMethod     For animation frames, a blending method to be used
    *                         when compositing this frame.
    * @param aBlendRect       For animation frames, if present, the subrect in
    *                         which @aBlendMethod applies. Outside of this
    *                         subrect, BlendMethod::OVER is always used.
-   * @param aFinalize        Finalize the underlying surface (e.g. so that it
-   *                         may be marked as read only if possible).
    */
   void Finish(Opacity aFrameOpacity = Opacity::SOME_TRANSPARENCY,
               DisposalMethod aDisposalMethod = DisposalMethod::KEEP,
               FrameTimeout aTimeout = FrameTimeout::FromRawMilliseconds(0),
               BlendMethod aBlendMethod = BlendMethod::OVER,
-              const Maybe<IntRect>& aBlendRect = Nothing(),
-              bool aFinalize = true);
+              const Maybe<IntRect>& aBlendRect = Nothing());
 
   /**
    * Mark this imgFrame as aborted. This informs the imgFrame that if it isn't
    * completely decoded now, it never will be.
    *
    * You must always call either Finish() or Abort() before releasing the last
    * RawAccessFrameRef pointing to an imgFrame.
    */
@@ -339,17 +336,16 @@ public:
 
   AnimationData GetAnimationData() const;
 
   bool GetCompositingFailed() const;
   void SetCompositingFailed(bool val);
 
   void SetOptimizable();
 
-  void FinalizeSurface();
   already_AddRefed<SourceSurface> GetSourceSurface();
 
   void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf, size_t& aHeapSizeOut,
                               size_t& aNonHeapSizeOut) const;
 
 private: // methods
 
   ~imgFrame();
@@ -360,17 +356,16 @@ private: // methods
 
   void AssertImageDataLocked() const;
 
   bool AreAllPixelsWritten() const;
   nsresult ImageUpdatedInternal(const nsIntRect& aUpdateRect);
   void GetImageDataInternal(uint8_t** aData, uint32_t* length) const;
   uint32_t GetImageBytesPerRow() const;
   uint32_t GetImageDataLength() const;
-  void FinalizeSurfaceInternal();
   already_AddRefed<SourceSurface> GetSourceSurfaceInternal();
 
   uint32_t PaletteDataLength() const
   {
     return mPaletteDepth ? (size_t(1) << mPaletteDepth) * sizeof(uint32_t)
                          : 0;
   }
 
@@ -617,14 +612,12 @@ public:
   }
 
 private:
   RawAccessFrameRef(const RawAccessFrameRef& aOther) = delete;
 
   RefPtr<imgFrame> mFrame;
 };
 
-void MarkSurfaceShared(gfx::SourceSurface* aSurface);
-
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_imgFrame_h