If we fail to contruct the Pdf Image Shader, mark the object as busted, and dn't try to remove it f…

src/pdf/SkPDFShader.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkPDFShader.h"
@@ skipping 426 matching lines @@
     SkTDArray<SkPDFObject*> fResources;
     SkAutoTDelete<const SkPDFShader::State> fState;
 
     SkPDFStream* makePSFunction(const SkString& psCode, SkPDFArray* domain);
 };
 
 class SkPDFImageShader : public SkPDFStream, public SkPDFShader {
 public:
     explicit SkPDFImageShader(SkPDFShader::State* state);
     virtual ~SkPDFImageShader() {
-        RemoveShader(this);
-        fResources.unrefAll();
+        if (!fFailedConstructor) {
+            RemoveShader(this);
+            fResources.unrefAll();
+        }
     }
 
-    virtual bool isValid() { return size() > 0; }
+    virtual bool isValid() { return !fFailedConstructor && size() > 0; }
 
     void getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
                       SkTSet<SkPDFObject*>* newResourceObjects) {
         GetResourcesHelper(&fResources.toArray(),
                            knownResourceObjects,
                            newResourceObjects);
     }
 
 private:
     SkTSet<SkPDFObject*> fResources;
     SkAutoTDelete<const SkPDFShader::State> fState;
+    bool fFailedConstructor;

[vandebo (ex-Chrome), 2013/07/23 15:09:50]

I don't think you need this field.  Just adding the isValid() call to the
destructor should be sufficient.

[edisonn, 2013/07/23 15:34:18]

Done.

 };
 
 SkPDFShader::SkPDFShader() {}
 
 // static
 void SkPDFShader::RemoveShader(SkPDFObject* shader) {
     SkAutoMutexAcquire lock(CanonicalShadersMutex());

[vandebo (ex-Chrome), 2013/07/23 15:09:50]

Instead of dropping the lock in the factory method, would it make sense to put
all of this inside an if(isValid()) block?

[edisonn, 2013/07/23 15:34:18]

I personally don't think so. We already have an assert that the shader is found
(line 475) so it seems to me that the function was wrote with the intend of
correct usage. I think we should preserve the original intend, unless we think
we should change the contract.

On 2013/07/23 15:09:50, vandebo wrote:

[vandebo (ex-Chrome), 2013/07/23 15:41:27]

I didn't notice that - that's even a better reason to change it to check for
valid first.  If it's not valid, then we will never insert it into the set of
canonical shaders.

     ShaderCanonicalEntry entry(shader, NULL);
     int index = CanonicalShaders().find(entry);
     SkASSERT(index >= 0);
     CanonicalShaders().removeShuffle(index);
 }
 
 // static
 SkPDFObject* SkPDFShader::GetPDFShader(const SkShader& shader,
                                        const SkMatrix& matrix,
                                        const SkIRect& surfaceBBox) {
@@ skipping 24 matching lines @@
             new SkPDFImageShader(shaderState.detach());
         valid = imageShader->isValid();
         result = imageShader;
     } else {
         SkPDFFunctionShader* functionShader =
             new SkPDFFunctionShader(shaderState.detach());
         valid = functionShader->isValid();
         result = functionShader;
     }
     if (!valid) {
+        // Release the lock, otherwise we end up calling RemoveShader that locks again,

[vandebo (ex-Chrome), 2013/07/23 15:09:50]

nit: please wrap at 80 cols; the rest of the file is 80 cols and it doesn't make
things any longer or any easier to read with the 100 col wrap.

[edisonn, 2013/07/23 15:34:18]

Done.

+        // and we end up with a freeze.
+        lock.release();
         delete result;
         return NULL;
     }
     entry.fPDFShader = result;
     CanonicalShaders().push(entry);
     return result;  // return the reference that came from new.
 }
 
 // static
 SkTDArray<SkPDFShader::ShaderCanonicalEntry>& SkPDFShader::CanonicalShaders() {
@@ skipping 128 matching lines @@
 
     SkPDFStream* function = makePSFunction(functionCode, domain.get());
     pdfShader->insert("Function", new SkPDFObjRef(function))->unref();
     fResources.push(function);  // Pass ownership to resource list.
 
     insertInt("PatternType", 2);
     insert("Matrix", SkPDFUtils::MatrixToArray(finalMatrix))->unref();
     insert("Shading", pdfShader.get());
 }
 
-SkPDFImageShader::SkPDFImageShader(SkPDFShader::State* state) : fState(state) {
+SkPDFImageShader::SkPDFImageShader(SkPDFShader::State* state) : fState(state)
+                                                              , fFailedConstructor(false) {
     fState.get()->fImage.lockPixels();
 
     SkMatrix finalMatrix = fState.get()->fCanvasTransform;
     finalMatrix.preConcat(fState.get()->fShaderTransform);
     SkRect surfaceBBox;
     surfaceBBox.set(fState.get()->fBBox);
     if (!transformBBox(finalMatrix, &surfaceBBox)) {
+        fFailedConstructor = true;
         return;
     }
 
     SkMatrix unflip;
     unflip.setTranslate(0, SkScalarRoundToScalar(surfaceBBox.height()));
     unflip.preScale(SK_Scalar1, -SK_Scalar1);
     SkISize size = SkISize::Make(SkScalarRound(surfaceBBox.width()),
                                  SkScalarRound(surfaceBBox.height()));
     SkPDFDevice pattern(size, size, unflip);
     SkCanvas canvas(&pattern);
@@ skipping 275 matching lines @@
         fPixelGeneration = fImage.getGenerationID();
     } else {
         fColorData.set(sk_malloc_throw(
                     fInfo.fColorCount * (sizeof(SkColor) + sizeof(SkScalar))));
         fInfo.fColors = reinterpret_cast<SkColor*>(fColorData.get());
         fInfo.fColorOffsets =
             reinterpret_cast<SkScalar*>(fInfo.fColors + fInfo.fColorCount);
         shader.asAGradient(&fInfo);
     }
 }