Trying to be consistent when members are allowed to be NULL

Trying to be consistent when members are allowed to be NULL

Got a few crashes running the fuzzer locally, all related to handling NULL members/parameters in an inconsistent way.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9bde31e95ddd82a4a923275bb01265740407dab1

[sugoi1, 2014-10-29 15:26:11]

sugoi@chromium.org changed reviewers:
+ senorblanco@google.com

[sugoi1, 2014-10-29 15:26:12]

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
File src/core/SkLocalMatrixShader.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
src/core/SkLocalMatrixShader.cpp:63: if (localMatrix.isIdentity() || !proxy) {
proxy may be NULL (see SkRectShaderImageFilter)

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
src/effects/SkColorFilterImageFilter.cpp:65: if (input && cf &&
cf->asColorMatrix(colorMatrix)
SkColorFilterImageFilter's constructor calls SkSafeRef() on cf, so having a NULL
cf is possible (although it will assert in Debug), so we should check for that
here.

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
src/effects/SkColorFilterImageFilter.cpp:144: SkSafeRef(fColorFilter);
SkColorFilterImageFilter's constructor and destructor call SkSafeRef() /
SkSafeUnref() on fColorFilter, so it should be the same here.

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
src/effects/SkRectShaderImageFilter.cpp:36: SkSafeRef(s);
This class' destructor calls SkSafeUnref(), so the constructor should call
SkSafeRef().

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
src/effects/SkRectShaderImageFilter.cpp:81:
SkSafeUnref(paint.setShader(SkShader::CreateLocalMatrixShader(fShader,
matrix)));
If fShader can be NULL, then SkShader::CreateLocalMatrixShader() may return
NULL.

[reed1, 2014-10-29 15:43:38]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-29 15:43:39]

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
File src/core/SkLocalMatrixShader.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
src/core/SkLocalMatrixShader.cpp:63: if (localMatrix.isIdentity() || !proxy) {
Would this be even clearer as:

if (NULL == proxy) {
    return NULL;
}
if (localMatrix.identity()) {
    return SkRef(proxy);
}

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
src/effects/SkColorFilterImageFilter.cpp:65: if (input && cf &&
cf->asColorMatrix(colorMatrix)
On 2014/10/29 15:26:12, sugoi1 wrote:
> SkColorFilterImageFilter's constructor calls SkSafeRef() on cf, so having a
NULL
> cf is possible (although it will assert in Debug), so we should check for that
> here.

Does SkColorFilterImageFilter do anything if there is no colorfilter? Why not
just

if (NULL == cf) {
    return NULL;
}

Part of the value of using factories (like Create) is that we can perform these
sorts of safety/optimization checks.

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
src/effects/SkRectShaderImageFilter.cpp:36: SkSafeRef(s);
How did we get called with a NULL. Is this guy only access via a factory (e.g.
Create)? If so, seems better to have that guy just return NULL.

[sugoi1, 2014-10-29 18:21:00]

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
File src/core/SkLocalMatrixShader.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/core/SkLocalMatrixShader...
src/core/SkLocalMatrixShader.cpp:63: if (localMatrix.isIdentity() || !proxy) {
On 2014/10/29 15:43:38, reed1 wrote:
> Would this be even clearer as:
> 
> if (NULL == proxy) {
>     return NULL;
> }
> if (localMatrix.identity()) {
>     return SkRef(proxy);
> }

Done.

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkColorFilterIma...
src/effects/SkColorFilterImageFilter.cpp:65: if (input && cf &&
cf->asColorMatrix(colorMatrix)
On 2014/10/29 15:43:38, reed1 wrote:
> On 2014/10/29 15:26:12, sugoi1 wrote:
> > SkColorFilterImageFilter's constructor calls SkSafeRef() on cf, so having a
> NULL
> > cf is possible (although it will assert in Debug), so we should check for
that
> > here.
> 
> Does SkColorFilterImageFilter do anything if there is no colorfilter? Why not
> just
> 
> if (NULL == cf) {
>     return NULL;
> }
> 
> Part of the value of using factories (like Create) is that we can perform
these
> sorts of safety/optimization checks.

Done.

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/1/src/effects/SkRectShaderImag...
src/effects/SkRectShaderImageFilter.cpp:36: SkSafeRef(s);
On 2014/10/29 15:43:39, reed1 wrote:
> How did we get called with a NULL. Is this guy only access via a factory (e.g.
> Create)? If so, seems better to have that guy just return NULL.

Done.

[reed1, 2014-10-29 18:27:08]

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkColorFilte...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:147: SkSafeRef(fColorFilter);
When can this ever be NULL?

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkRectShader...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkRectShader...
src/effects/SkRectShaderImageFilter.cpp:36: SkSafeRef(s);
When can this be NULL?

[sugoi1, 2014-10-29 18:30:58]

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkColorFilte...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:147: SkSafeRef(fColorFilter);
On 2014/10/29 18:27:08, reed1 wrote:
> When can this ever be NULL?

I was just trying to be consistent with constructor/destructor, I'll change
those instead.

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkRectShader...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/20001/src/effects/SkRectShader...
src/effects/SkRectShaderImageFilter.cpp:36: SkSafeRef(s);
On 2014/10/29 18:27:08, reed1 wrote:
> When can this be NULL?

I was just trying to have the constructor and destructor to be consistent with
each other, I'll change the destructor instead.

[sugoi1, 2014-10-29 18:35:37]

Removed unnecessary SkSafeRef()/SkSafeUnref() calls

[reed1, 2014-10-29 19:55:00]

lgtm

https://codereview.chromium.org/675013003/diff/40001/src/effects/SkColorFilte...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/40001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:84: : INHERITED(1, &input, cropRect,
uniqueID), fColorFilter(cf) {
suggestion nit: could just say fColorFilter(SkRef(cf)) { ...

https://codereview.chromium.org/675013003/diff/40001/src/effects/SkRectShader...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/675013003/diff/40001/src/effects/SkRectShader...
src/effects/SkRectShaderImageFilter.cpp:34: , fShader(s) {
suggestion nit: , fShader(SkRef(s)) {

[Stephen White, 2014-10-29 20:22:51]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-10-29 20:22:51]

LGTM

[sugoi1, 2014-10-30 20:56:51]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-10-30 20:57:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675013003/60001

[commit-bot: I haz the power, 2014-10-30 21:05:19]

Committed patchset #4 (id:60001) as 9bde31e95ddd82a4a923275bb01265740407dab1