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 (isValid()) {
+            RemoveShader(this);
+            fResources.unrefAll();

[vandebo (ex-Chrome), 2013/07/23 17:29:00]

If it isn't valid, fResources should be empty, but it doesn't hurt to unref
anything that's in there, so move it out of the conditional.

[edisonn, 2013/07/23 17:35:39]

Done.

+        }
     }
 
     virtual bool isValid() { return size() > 0; }
 
     void getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
                       SkTSet<SkPDFObject*>* newResourceObjects) {
         GetResourcesHelper(&fResources.toArray(),
                            knownResourceObjects,
                            newResourceObjects);
     }
@@ skipping 45 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

[vandebo (ex-Chrome), 2013/07/23 17:29:00]

With the isValid() check in the destructor, you don't need this.

[edisonn, 2013/07/23 17:35:39]

true for image shader, but I am not 100% for function shader, which has 5 early
returns from the constructor ...

[vandebo (ex-Chrome), 2013/07/23 17:47:33]

All the early exits are before fResources is populated, which is what function
shader's isValid method looks at.

But also logically, it should not be in the canonical shader list if it is not
valid.

[edisonn, 2013/07/23 17:54:13]

done (function shader already has the valid check in destructor)
On 2013/07/23 17:47:33, vandebo wrote:

+        // locks again, 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 431 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);
     }
 }