Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.

BUG=646654
Committed: https://crrev.com/ce494afba26d386e562b8e8d84ef707d5c54bfc3
Cr-Commit-Position: refs/heads/master@{#418927}

[xlai (Olivia), 2016-09-14 17:37:22]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2016-09-14 17:39:28]

Description was changed from

==========
Assing identity matrix to m_previousLocalMatrix in ImagePattern

It's possible that the m_previousLocalMatrix has not been properly constructed
yet in ImagePattern constructor, thereby causing a crash in directly setting
it to identiy. Instead, create another identity matrix and assign to
m_previousLocalmatrix.

BUG=646654
==========

to

==========
Assgning identity matrix to m_previousLocalMatrix in ImagePattern

It's possible that the m_previousLocalMatrix has not been properly constructed
yet in ImagePattern constructor, thereby causing a crash in directly setting
it to identiy. Instead, create another identity matrix and assign to
m_previousLocalmatrix.

BUG=646654
==========

[Justin Novosad, 2016-09-14 18:05:08]

https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:26:
matrix.setIdentity();
I don't understand what this is supposed to fix. The constructors of class
members are always invoked prior to the class constructor.  Do you have a test
that demonstrates what this fixes?

[Justin Novosad, 2016-09-14 18:06:33]

I just looked at the crash report.  What I think is happening is that |image| is
nullptr, and the initializer code is trying to call
image->imageForCurrentFrame(), which crashes

[Justin Novosad, 2016-09-14 18:07:45]

https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:18: return
adoptRef(new ImagePattern(std::move(image), repeatMode));
Perhaps add a null check here?

[xlai (Olivia), 2016-09-14 18:54:53]

Description was changed from

==========
Assgning identity matrix to m_previousLocalMatrix in ImagePattern

It's possible that the m_previousLocalMatrix has not been properly constructed
yet in ImagePattern constructor, thereby causing a crash in directly setting
it to identiy. Instead, create another identity matrix and assign to
m_previousLocalmatrix.

BUG=646654
==========

to

==========
Correct SourImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

[xlai (Olivia), 2016-09-14 18:55:22]

Description was changed from

==========
Correct SourImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

to

==========
Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

[xlai (Olivia), 2016-09-14 18:58:45]

Redo the CL and changed title and description.

Instead of doing an ad-hoc null check, I think it's better to find the root
cause of why the image is a nullptr
and it only happens in OffscreenCanvas. I traced the creation of ImagePattern
all the way back and find that
in BaseRenderingContext2D::createPattern, if the CanvasImageSource is correctly
set to InvalidSourceImageStatus,
then the image would be set to Image::nullImage() instead of nullptr. So
something was wrong when imageSource->
getSourceImageForCanvas is called. I then find that OffscreenCanvas's override
function has a possibility of
returning nullptr image yet set the SourceImageStatus to be
NormalSourceImageStatus.



On 2016/09/14 18:07:45, Justin Novosad wrote:
>
https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (right):
> 
>
https://codereview.chromium.org/2344573002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:18: return
> adoptRef(new ImagePattern(std::move(image), repeatMode));
> Perhaps add a null check here?

[Justin Novosad, 2016-09-14 20:30:31]

Great fix, but please add a test

[xlai (Olivia), 2016-09-15 16:28:33]

Using the ClusterFuzz use case to write a minimized test case. This test will
crash in master branch but pass after the current fix.

[Justin Novosad, 2016-09-15 17:14:46]

lgtm

[xlai (Olivia), 2016-09-15 17:56:22]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-09-15 17:56:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 19:05:32]

Description was changed from

==========
Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

to

==========
Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

[commit-bot: I haz the power, 2016-09-15 19:05:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-15 19:07:44]

Description was changed from

==========
Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function 
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.


BUG=646654
==========

to

==========
Correct SourceImageStatus in OffscreenCanvas::getSourceImageForCanvas

The crash in constructor of ImagePattern may be due to the fact that
image passed in as its constructor argument is nullptr. By tracking
all the call trace backwards, we find the image creation originates
from BaseRenderingContext2D::createPattern, where the SourceImageStatus
indicates what corresponding action to do. But this SourceImageStatus
was wrongly set for OffscreenCanvas, in particular, in the function
OffscreenCanvas::getSourceImageForCanvas. This CL corrects it and makes
sure that if the image is null, BaseRenderingContext2D::createPattern
will return Image::nullImage() instead of nullptr.

BUG=646654

Committed: https://crrev.com/ce494afba26d386e562b8e8d84ef707d5c54bfc3
Cr-Commit-Position: refs/heads/master@{#418927}
==========

[commit-bot: I haz the power, 2016-09-15 19:07:45]

Patchset 4 (id:??) landed as
https://crrev.com/ce494afba26d386e562b8e8d84ef707d5c54bfc3
Cr-Commit-Position: refs/heads/master@{#418927}