Bug 50151 – Crash in RenderBox::paintMaskImages due to a mask without an associated image

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Berend-Jan Wever]

Created attachment 75020 [details]
Repro

Repro:
<style>
  *{
    -webkit-mask-image:none,none,url(x);
  }
</style>
id:             chrome.dll!WebCore::RenderBox::paintMaskImages ReadAV@NULL (f12ffd1e9345f0da79fdc0164c287064)
description:    Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::RenderBox::paintMaskImages
application:    Chromium 9.0.596.0
stack:          chrome.dll!WebCore::RenderBox::paintMaskImages
                chrome.dll!WebCore::RenderBox::paintMask
                chrome.dll!WebCore::RenderBlock::paintObject
                chrome.dll!WebCore::RenderBlock::paint
                chrome.dll!WebCore::RenderLayer::paintLayer
                chrome.dll!WebCore::RenderLayer::paintList
                chrome.dll!WebCore::RenderLayer::paintLayer
                chrome.dll!WebCore::RenderLayer::paintList
                chrome.dll!WebCore::RenderLayer::paintLayer
                chrome.dll!WebCore::RenderLayer::paint
                chrome.dll!WebCore::FrameView::paintContents
                chrome.dll!WebCore::ScrollView::paint
                chrome.dll!WebKit::WebFrameImpl::paintWithContext
                chrome.dll!WebKit::WebFrameImpl::paint
                chrome.dll!RenderWidget::PaintRect
                chrome.dll!RenderWidget::DoDeferredUpdate
                chrome.dll!RenderWidget::CallDoDeferredUpdate
                chrome.dll!MessageLoop::RunTask
                chrome.dll!MessageLoop::DoWork
                chrome.dll!base::MessagePumpDefault::Run
                chrome.dll!MessageLoop::RunInternal
                ...
Exact stack can differ: it appears that if you put element tags in the repro, they vary depending on what element gets rendered first.

[Julien Chaffraix]

FYI, the test case does not crash under DRT but definitely does under the browser (tested on Qt/Linux and Chromium/Linux)

[Abhishek Arya]

Testcase1:
<style>
.f {

 -webkit-mask:-webkit-gradient(linear, left top, left bottom, from(#E7E7E7), to(#CFCFCF));</style>

<table
<tr class="f">
<td>

Testcase2:
<style>

td { border-spacing : use-script; -webkit-mask-image : url(red_transparent.gif);</style>
AA000A00AAA00<table onmousewheel="entrybar"
<tr><td>

Testcase3:
<style>
        .box {

 display: table-footer-group; -webkit-mask-box-image: url("bogus.png");</style>
     
<div class="box"><figure>

[Julien Chaffraix]

(In reply to comment #1)
> FYI, the test case does not crash under DRT but definitely does under the browser (tested on Qt/Linux and Chromium/Linux)

I was wrong here: you need to dump the pixels if you want the test to crash (which it does). Sorry for the noise. I have a WIP fix, taking the bug.

[Julien Chaffraix]

There is actually 2 bugs covered by this one (which explains the different stacktrace you were getting).

One is related to FillLayer usage in paintMaskImage (and is covered by Berend-Jan's test case (at least on my machine)).

The other one is that paintMaskImage could be called with a GraphicsContext with paintingDisabled() set (called from FrameView::updateControlTints). This bug is partly covered by Abhishek's test cases.

I will fix the first one as part of this bug and file a follow-up bug for the second one as it will need to add a reliable way to test under DRT.

[Julien Chaffraix]

Created attachment 107362 [details]
Proposed fix: patch paintMaskImages to properly check the image(), also avoid calling hasImage() in a loop as it is already looping.

[Simon Fraser (smfr)]

Comment on attachment 107362 [details]
Proposed fix: patch paintMaskImages to properly check the image(), also avoid calling hasImage() in a loop as it is already looping.

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

> LayoutTests/fast/css/empty-webkit-mask-crash.html:4
> +    // We need to dump the image, we don't care about the layout information.

It would be nice if the text content said "This test should not crash".

> Source/WebCore/ChangeLog:3
> +        chrome.dll!WebCore::RenderBox::paintMaskImages ReadAV@NULL (various hashes)

Retitle the bug to reflect the actual issue, please.

> Source/WebCore/ChangeLog:10
> +        The crash stems from the fact that FillLayer::hasImage would over the linked list

missing verb (would __ over)

[Julien Chaffraix]

Comment on attachment 107362 [details]
Proposed fix: patch paintMaskImages to properly check the image(), also avoid calling hasImage() in a loop as it is already looping.

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

>> LayoutTests/fast/css/empty-webkit-mask-crash.html:4
>> +    // We need to dump the image, we don't care about the layout information.
> 
> It would be nice if the text content said "This test should not crash".

There is a comment above that mention the passing condition. I did not want to have a platform-specific output as we really don't care about it and I don't see how we can do what you want in a cross-platform fashion. If you see one, I am all for it too!

>> Source/WebCore/ChangeLog:10
>> +        The crash stems from the fact that FillLayer::hasImage would over the linked list
> 
> missing verb (would __ over)

Oh! __ = walk :-)

[Simon Fraser (smfr)]

(In reply to comment #7)
> There is a comment above that mention the passing condition. I did not want to have a platform-specific output as we really don't care about it

There isn't a problem with platform-specific output, since this is a dumpAsText test.

[Julien Chaffraix]

Created attachment 107531 [details]
Patch for landing

[Julien Chaffraix]

Created attachment 107547 [details]
Patch for landing

[Julien Chaffraix]

(In reply to comment #8)
> (In reply to comment #7)
> > There is a comment above that mention the passing condition. I did not want to have a platform-specific output as we really don't care about it
> 
> There isn't a problem with platform-specific output, since this is a dumpAsText test.

FYI, I was confused with what dumpAsText(true) would dump. After I was corrected, I added a mention of the bug and condition to pass in text output for clarity.

[WebKit Review Bot]

Comment on attachment 107547 [details]
Patch for landing

Clearing flags on attachment: 107547

Committed r95235: <http://trac.webkit.org/changeset/95235>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.