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

Index: src/pdf/SkPDFShader.cpp
===================================================================
--- src/pdf/SkPDFShader.cpp	(revision 10235)
+++ src/pdf/SkPDFShader.cpp	(working copy)
@@ -444,8 +444,10 @@
 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; }
@@ -511,6 +513,9 @@
         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;
     }