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

If we fail to contruct the Pdf Image Shader, mark the object as busted, and dn't try to remove it from shaders list. Also, when we delete invalid shaders, remove the lock, otherwise we freeze (see CanonicalShadersMutex usage).

R=vandebo@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=10290

[edisonn, 2013-07-23 13:20:12]

BTW, we found this bug by running render_pdf on all the skps Ravi collected.
Thanks!

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

On 2013/07/23 13:20:12, edisonn wrote:
> BTW, we found this bug by running render_pdf on all the skps Ravi collected.
> Thanks!

Cool, which skp?  It's strange that the matrix isn't invertible, it might be
interesting to see what's going on.

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:465: bool fFailedConstructor;
I don't think you need this field.  Just adding the isValid() call to the
destructor should be sufficient.

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:472: SkAutoMutexAcquire lock(CanonicalShadersMutex());
Instead of dropping the lock in the factory method, would it make sense to put
all of this inside an if(isValid()) block?

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:517: // Release the lock, otherwise we end up calling
RemoveShader that locks again,
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]

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:465: bool fFailedConstructor;
On 2013/07/23 15:09:50, vandebo wrote:
> I don't think you need this field.  Just adding the isValid() call to the
> destructor should be sufficient.

Done.

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:472: SkAutoMutexAcquire lock(CanonicalShadersMutex());
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:
> Instead of dropping the lock in the factory method, would it make sense to put
> all of this inside an if(isValid()) block?

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:517: // Release the lock, otherwise we end up calling
RemoveShader that locks again,
On 2013/07/23 15:09:50, vandebo wrote:
> 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.

Done.

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

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:472: SkAutoMutexAcquire lock(CanonicalShadersMutex());
On 2013/07/23 15:34:18, edisonn wrote:
> 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.

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.

> 
> On 2013/07/23 15:09:50, vandebo wrote:
> > Instead of dropping the lock in the factory method, would it make sense to
put
> > all of this inside an if(isValid()) block?
>

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

Sorry, missed that you had a new patch set.

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:449: fResources.unrefAll();
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.

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:516: // Release the lock, otherwise we end up calling
RemoveShader that
With the isValid() check in the destructor, you don't need this.

[edisonn, 2013-07-23 17:35:38]

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:449: fResources.unrefAll();
On 2013/07/23 17:29:00, vandebo wrote:
> 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.

Done.

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:516: // Release the lock, otherwise we end up calling
RemoveShader that
On 2013/07/23 17:29:00, vandebo wrote:
> With the isValid() check in the destructor, you don't need this.

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]

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:516: // Release the lock, otherwise we end up calling
RemoveShader that
On 2013/07/23 17:35:39, edisonn wrote:
> On 2013/07/23 17:29:00, vandebo wrote:
> > With the isValid() check in the destructor, you don't need this.
> 
> true for image shader, but I am not 100% for function shader, which has 5
early
> returns from the constructor ...

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]

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/19509005/diff/6001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:516: // Release the lock, otherwise we end up calling
RemoveShader that
done (function shader already has the valid check in destructor)
On 2013/07/23 17:47:33, vandebo wrote:
> On 2013/07/23 17:35:39, edisonn wrote:
> > On 2013/07/23 17:29:00, vandebo wrote:
> > > With the isValid() check in the destructor, you don't need this.
> > 
> > true for image shader, but I am not 100% for function shader, which has 5
> early
> > returns from the constructor ...
> 
> 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.

[vandebo (ex-Chrome), 2013-07-23 18:14:24]

LGTM

[edisonn, 2013-07-23 19:45:40]

Committed patchset #5 manually as r10290.