Bug 118686 – Dereference null pointer crash in Length::decrementCalculatedRef()

• Component: CSS
• Status: RESOLVED FIXED

[Jacky Jiang]

I got this crash once and do not know how to make a test case here so far.

Program terminated with signal 11, Segmentation fault. 
#0  hasOneRef (this=0x0) at /home/jacky/dev/webkit/Source/WTF/wtf/RefCounted.h:75 
75	        return m_refCount == 1; 
(gdb) bt 
#0  hasOneRef (this=0x0) at /home/jacky/dev/webkit/Source/WTF/wtf/RefCounted.h:75 
#1  WebCore::Length::decrementCalculatedRef (this=0x77eba4d0) at /home/jacky/dev/webkit/Source/WebCore/platform/Length.cpp:234 
#2  0x7bdd9166 in ~Length (this=0x77eba4d0, __in_chrg=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/platform/Length.h:101 
#3  WebCore::TranslateTransformOperation::blend (this=<optimized out>, from=<optimized out>, progress=0, blendToIdentity=true) 
    at /home/jacky/dev/webkit/Source/WebCore/platform/graphics/transforms/TranslateTransformOperation.cpp:36 
#4  0x7bdd4d80 in WebCore::TransformOperations::blendByMatchingOperations (this=0x79d9bb7c, from=..., progress=@0x77eba608: 0) 
    at /home/jacky/dev/webkit/Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:78 
#5  0x7bd6e76e in blendFunc (progress=0, to=..., from=..., anim=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/page/animation/CSSPropertyAnimation.cpp:127 
#6  WebCore::PropertyWrapperAcceleratedTransform::blend (this=<optimized out>, anim=<optimized out>, dst=0x7ccdecb0, a=<optimized out>, b=0x7c870c20, progress=0) 
    at /home/jacky/dev/webkit/Source/WebCore/page/animation/CSSPropertyAnimation.cpp:493 
#7  0x7bd665de in WebCore::CSSPropertyAnimation::blendProperties (anim=0x7ae7de20, prop=<optimized out>, dst=0x7ccdecb0, a=<optimized out>, b=0x7c870c20, progress=0) 
    at /home/jacky/dev/webkit/Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1268 
#8  0x7bd6f212 in WebCore::ImplicitAnimation::animate (this=0x7ae7de20, targetStyle=0x7c870c20, animatedStyle=...) 
    at /home/jacky/dev/webkit/Source/WebCore/page/animation/ImplicitAnimation.cpp:81 
#9  0x7bd6101c in WebCore::CompositeAnimation::animate (this=0x7d2bc9e0, renderer=0x7d46c708, currentStyle=0x7cd19710, targetStyle=0x7c870c20) 
    at /home/jacky/dev/webkit/Source/WebCore/page/animation/CompositeAnimation.cpp:292 
#10 0x7bd5e398 in WebCore::AnimationController::updateAnimations (this=0x79d8ef20, renderer=0x7d46c708, newStyle=0x7c870c20) 
    at /home/jacky/dev/webkit/Source/WebCore/page/animation/AnimationController.cpp:523 
#11 0x7bed86b8 in WebCore::RenderObject::setAnimatableStyle (this=0x7d46c708, style=...) at /home/jacky/dev/webkit/Source/WebCore/rendering/RenderObject.cpp:1710 
#12 0x7ba69844 in WebCore::Element::recalcStyle (this=0x7c872958, change=WebCore::Node::NoChange) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1395 
#13 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7acaa710, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#14 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7aa55bd8, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#15 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7aa55b90, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#16 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7aa55998, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#17 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7aa524d0, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#18 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7aa50a70, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#19 0x7ba69914 in WebCore::Element::recalcStyle (this=0x7a123b00, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#20 0x7ba69914 in WebCore::Element::recalcStyle (this=0x79dbde60, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:1448 
#21 0x7ba4e950 in recalcStyle (change=<optimized out>, this=0x79f9c8a0) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1840 
#22 WebCore::Document::recalcStyle (this=0x79f9c8a0, change=<optimized out>) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1777 
#23 0x7ba4eba6 in updateStyleIfNeeded (this=0x79f9c8a0) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1885 
#24 WebCore::Document::updateStyleIfNeeded (this=0x79f9c8a0) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1874 
#25 0x7ba4f5b2 in WebCore::Document::updateLayout (this=0x79f9c8a0) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1916 
#26 0x7ba50e6c in WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x79f9c8a0) at /home/jacky/dev/webkit/Source/WebCore/dom/Document.cpp:1954 
#27 0x7ba64974 in WebCore::Element::offsetWidth (this=0x7c872958) at /home/jacky/dev/webkit/Source/WebCore/dom/Element.cpp:517

[Jacky Jiang]

f 1
p *this 
$24 = {{m_intValue = 0, m_floatValue = 0}, m_quirk = false, m_type = 10 '\n', m_isFloat = false} 
(gdb) f 3

[Mike Lawther]

Is this repeatable? If so, can you please attach the page source that's causing the crash. It looks like you're blending transforms where one of the transform coeffs is defined using calc?

[Jacky Jiang]

(In reply to comment #2)
> Is this repeatable? If so, can you please attach the page source that's causing the crash. It looks like you're blending transforms where one of the transform coeffs is defined using calc?

I only managed to reproduce once somehow. I am trying to make a test case base on the code path and see if that can reproduce the crash. 
Yes,  m_x.type(), m_y.type() or m_y.type() must be Calculated, so that it could ASSERT on debug mode and crashed on release mode when the temporary object Length(m_x.type()) went out of the scope and deference the null CalculationValue pointer.  See the code here:
return TranslateTransformOperation::create(Length(m_x.type()).blend(m_x, progress), 
                                                  Length(m_y.type()).blend(m_y, progress), 
                                                   Length(m_z.type()).blend(m_z, progress), m_type);

[Jacky Jiang]

Created attachment 206908 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 206908 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Test: css3/calc/transition-transform-translate-crash.html

This test name should have "calc" in it somewhere.

> LayoutTests/css3/calc/transition-transform-translate-crash.html:6
> +    div#div2 {

You don't need to specify the tag name if you are also using an id.

You could do divs at the same time with a class on the body or a container div.

> LayoutTests/css3/calc/transition-transform-translate-crash.html:19
> +        testRunner.waitUntilDone();

You don't need waitUntilDone()/notifyDone() unless the test also uses setTimeout().

[Jacky Jiang]

Comment on attachment 206908 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        Test: css3/calc/transition-transform-translate-crash.html
> 
> This test name should have "calc" in it somewhere.

What about transition-transform-translate-calculated-length-crash.html?

>> LayoutTests/css3/calc/transition-transform-translate-crash.html:6
>> +    div#div2 {
> 
> You don't need to specify the tag name if you are also using an id.
> 
> You could do divs at the same time with a class on the body or a container div.

div2 will need both -webkit-transition:-webkit-transform 50ms; and -webkit-transform:translate(-webkit-calc(10% + 10px)); which is different than div1.   How can you make one class for two different divs? What about div2?

>> LayoutTests/css3/calc/transition-transform-translate-crash.html:19
>> +        testRunner.waitUntilDone();
> 
> You don't need waitUntilDone()/notifyDone() unless the test also uses setTimeout().

Oops, you are right.

[Jacky Jiang]

Comment on attachment 206908 [details]
Patch

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

>>> LayoutTests/css3/calc/transition-transform-translate-crash.html:6
>>> +    div#div2 {
>> 
>> div2 will need both -webkit-transition:-webkit-transform 50ms; and -webkit-transform:translate(-webkit-calc(10% + 10px)); which is different than div1.   How can you make one class for two different divs? What about div2?
> 
> You don't need to specify the tag name if you are also using an id.
> 
> You could do divs at the same time with a class on the body or a container div.

You are right that I don't need specify the tag name, sorry.

[Simon Fraser (smfr)]

(In reply to comment #6)
> (From update of attachment 206908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206908&action=review
> 
> >> Source/WebCore/ChangeLog:17
> >> +        Test: css3/calc/transition-transform-translate-crash.html
> > 
> > This test name should have "calc" in it somewhere.
> 
> What about transition-transform-translate-calculated-length-crash.html?

Oops, I didn't realize it was under calc/. I think it should be under transitions/.  transition-transform-translate-calculated-length-crash.html is good.

[Jacky Jiang]

Comment on attachment 206908 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:17
>>>> +        Test: css3/calc/transition-transform-translate-crash.html
>>> 
>>> Oops, I didn't realize it was under calc/. I think it should be under transitions/.  transition-transform-translate-calculated-length-crash.html is good.
>> 
>> What about transition-transform-translate-calculated-length-crash.html?
> 
> This test name should have "calc" in it somewhere.

OK,  I will move the test to transitions/ and update the patch, thanks!

[Jacky Jiang]

Created attachment 206911 [details]
Patch

[Build Bot]

Comment on attachment 206911 [details]
Patch

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

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206928 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Jacky Jiang]

Comment on attachment 206911 [details]
Patch

LayoutTests/fullscreen/full-screen-iframe-with-max-width-height.html has been run successfully locally on Mac 10.7. I am not sure how could that run into that failure on Mac OS X 10.8.3. 
The failure of the test full-screen-iframe-with-max-width-height.html shows "FAIL: Timed out waiting for notifyDone to be called"   and also I don't see the relationship between this test and my patch.  So try again to commit.

[WebKit Commit Bot]

Comment on attachment 206911 [details]
Patch

Clearing flags on attachment: 206911

Committed r152825: <http://trac.webkit.org/changeset/152825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Lawther]

Belatedly - the webkit prefix on calc is now deprecated, all new uses should be unprefixed.