Bug 22132 - All calls of ImageBuffer::create and ImageBuffer::copyImage should be null checked
Summary: All calls of ImageBuffer::create and ImageBuffer::copyImage should be null ch...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-07 15:53 PST by Brett Wilson (Google)
Modified: 2015-06-19 16:36 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.43 KB, patch)
2015-06-19 09:32 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2015-06-19 10:25 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2015-06-19 10:26 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v3 (Corrects merge conflict) (13.04 KB, patch)
2015-06-19 10:53 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (763.84 KB, application/zip)
2015-06-19 11:25 PDT, Build Bot
no flags Details
Patch v3 (Test Fix) (13.25 KB, patch)
2015-06-19 12:23 PDT, Brent Fulgham
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2008-11-07 15:53:34 PST
Some calls of this function do not check the return value, or ASSERT that the return value is non-null. This can fail for a variety of legitimate reasons (the bitmap could be quite large in some cases, and there may not be enough resources to allocate it).

One example is CanvasRenderingContext2D::drawTextInternal which should just return if the call fails.
Comment 1 Brent Fulgham 2015-06-19 09:15:09 PDT
This was the cause of Bug 146147.
Comment 2 Brent Fulgham 2015-06-19 09:32:11 PDT
Created attachment 255193 [details]
Patch
Comment 3 zalan 2015-06-19 10:04:42 PDT
Comment on attachment 255193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255193&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:635
>          m_cachedImage = buffer->copyImage(DontCopyBackingStore, Unscaled);
> +        ASSERT(m_cachedImage);

nullptr is a valid return value of ImageBuffer::copyImage().
Comment 4 Brent Fulgham 2015-06-19 10:25:17 PDT
Created attachment 255196 [details]
Patch
Comment 5 Brent Fulgham 2015-06-19 10:26:22 PDT
Created attachment 255197 [details]
Patch
Comment 6 Brent Fulgham 2015-06-19 10:53:09 PDT
Created attachment 255198 [details]
Patch v3 (Corrects merge conflict)
Comment 7 Build Bot 2015-06-19 11:25:53 PDT
Comment on attachment 255198 [details]
Patch v3 (Corrects merge conflict)

Attachment 255198 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4611317411348480

New failing tests:
css3/masking/mask-repeat-space-padding.html
fast/borders/border-image-repeat-stretch.html
css3/blending/background-blend-mode-tiled-layers.html
css3/blending/background-blend-mode-data-uri-svg-image.html
css3/masking/mask-svg-no-fragmentId-tiled.html
css3/masking/mask-base64.html
Comment 8 Build Bot 2015-06-19 11:25:56 PDT
Created attachment 255201 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 9 Darin Adler 2015-06-19 11:35:38 PDT
Comment on attachment 255198 [details]
Patch v3 (Corrects merge conflict)

View in context: https://bugs.webkit.org/attachment.cgi?id=255198&action=review

Looks like this created some test failures. Please investigate why.

> Source/WebCore/platform/graphics/filters/FETile.cpp:76
> +    if (!pattern->tileImage())
> +        return;

I think it’s better to check the image for null before creating a Pattern with it. In the future, we might want to change Pattern to require a non-null image.
Comment 10 Brent Fulgham 2015-06-19 11:37:19 PDT
(In reply to comment #9)
> Comment on attachment 255198 [details]
> Patch v3 (Corrects merge conflict)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255198&action=review
> 
> Looks like this created some test failures. Please investigate why.

Yes! I had flipped the null check in one case. I'll be uploading a new patch soon.

> > Source/WebCore/platform/graphics/filters/FETile.cpp:76
> > +    if (!pattern->tileImage())
> > +        return;
> 
> I think it’s better to check the image for null before creating a Pattern
> with it. In the future, we might want to change Pattern to require a
> non-null image.

Ok.
Comment 11 Brent Fulgham 2015-06-19 12:23:21 PDT
Created attachment 255212 [details]
Patch v3 (Test Fix)
Comment 12 Brent Fulgham 2015-06-19 12:24:27 PDT
Comment on attachment 255212 [details]
Patch v3 (Test Fix)

Updating with correction for tests. Waiting for EWS bots to review.
Comment 13 Brent Fulgham 2015-06-19 13:46:35 PDT
Committed r185766: <http://trac.webkit.org/changeset/185766>
Comment 14 Darin Adler 2015-06-19 16:12:20 PDT
Comment on attachment 255212 [details]
Patch v3 (Test Fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=255212&action=review

> Source/WebCore/platform/graphics/filters/FETile.cpp:78
> +    auto pattern = Pattern::create(tileImageCopy, true, true);

Should be WTF::move(tileImageCopy), I think.
Comment 15 Brent Fulgham 2015-06-19 16:30:27 PDT
(In reply to comment #14)
> Comment on attachment 255212 [details]
> Patch v3 (Test Fix)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255212&action=review
> 
> > Source/WebCore/platform/graphics/filters/FETile.cpp:78
> > +    auto pattern = Pattern::create(tileImageCopy, true, true);
> 
> Should be WTF::move(tileImageCopy), I think.

I'll checkin a fix. Thanks for catching that.
Comment 16 Brent Fulgham 2015-06-19 16:36:19 PDT
Follow-up fix added in r185776. <https://trac.webkit.org/r185776>