Bug 89148 – Null-pointer crash when a derived color like -webkit-activelink is set in a gradient stop

• Component: CSS
• Status: RESOLVED FIXED

CSSGradientValue does not resolve colors until paint time when the corresponding element is not available.

As reported at http://crbug.com/85080

Created attachment 147709 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147709 [details]
Patch

Attachment 147709 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12966276

New failing tests:
fast/gradients/generated-gradients.html
fast/css/linear-gradient-currentcolor.html

[WebKit Review Bot]

Created attachment 147757 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 148054 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 148054 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:108
> +PassRefPtr<CSSGradientValue> CSSGradientValue::resolveStyles(StyleResolver* styleResolver)

It's a bit odd that a method called resolveStyles resturns a gradient. Maybe gradientWithStylesResolved()?

It's also odd that this method changes |this| yet returns a clone.

> Source/WebCore/css/CSSGradientValue.cpp:112
> +        derived = derived || styleResolver->isElementDerivedColor(m_stops[i].m_color.get());

derived |= 
Putting m_stops[i].m_color in a local variable would be a minor win here.

> Source/WebCore/css/StyleResolver.h:224
> +    static bool isElementDerivedColor(CSSPrimitiveValue*);

This name is hard to read. Is it "is element-derived color", or "is element derived-color"? It's also not being called on a color-related class.

Created attachment 152689 [details]
Patch

Thanks for the review, please take another look.

(In reply to comment #5)
> It's a bit odd that a method called resolveStyles resturns a gradient. Maybe gradientWithStylesResolved()?

Done.

> It's also odd that this method changes |this| yet returns a clone.

Yes, but this seems a little simpler than the alternatives that I could see.

> > Source/WebCore/css/CSSGradientValue.cpp:112
> > +        derived = derived || styleResolver->isElementDerivedColor(m_stops[i].m_color.get());
> 
> derived |= 
> Putting m_stops[i].m_color in a local variable would be a minor win here.
> 
> > Source/WebCore/css/StyleResolver.h:224
> > +    static bool isElementDerivedColor(CSSPrimitiveValue*);
> 
> This name is hard to read. Is it "is element-derived color", or "is element derived-color"? It's also not being called on a color-related class.

Renamed.

[Simon Fraser (smfr)]

(In reply to comment #7)
> Thanks for the review, please take another look.
> 
> (In reply to comment #5)
> > It's a bit odd that a method called resolveStyles resturns a gradient. Maybe gradientWithStylesResolved()?
> 
> Done.
> 
> > It's also odd that this method changes |this| yet returns a clone.
> 
> Yes, but this seems a little simpler than the alternatives that I could see.

Why not see if any of the color stops need to be derived, and if so clone and modify, otherwise return |this|?

Created attachment 153159 [details]
Patch

(In reply to comment #8)
> > > It's also odd that this method changes |this| yet returns a clone.
> > 
> > Yes, but this seems a little simpler than the alternatives that I could see.
> 
> Why not see if any of the color stops need to be derived, and if so clone and modify, otherwise return |this|?

Done.

[Simon Fraser (smfr)]

Comment on attachment 153159 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:441
> +        if (StyleResolver::colorFromPrimitiveValueIsDerivedFromElement(stop.m_color.get()))

Shame that we do this test here as well as in gradientWithStylesResolved(). Maybe we should just cache this state?

> Source/WebCore/css/StyleResolver.cpp:3372
> +                    if (item->isGradientValue()) {
> +                        m_style->setContent(StyleGeneratedImage::create(static_cast<CSSGradientValue*>(item)->gradientWithStylesResolved(this).get()), didSet);
> +                        didSet = true;
> +                    } else {
> +                        m_style->setContent(StyleGeneratedImage::create(static_cast<CSSImageGeneratorValue*>(item)), didSet);
> +                        didSet = true;
> +                    }

didSet is true for both branches, so not sure why you have to do that inside the conditional.

Created attachment 153716 [details]
Patch

(In reply to comment #11)
> > Source/WebCore/css/CSSGradientValue.cpp:441
> > +        if (StyleResolver::colorFromPrimitiveValueIsDerivedFromElement(stop.m_color.get()))
> 
> Shame that we do this test here as well as in gradientWithStylesResolved(). Maybe we should just cache this state?

Done.


> didSet is true for both branches, so not sure why you have to do that inside the conditional.

Fixed.

[WebKit Review Bot]

Comment on attachment 153716 [details]
Patch

Attachment 153716 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13324096

New failing tests:
transitions/equivalent-background-image-no-transition.html

[WebKit Review Bot]

Created attachment 153720 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 153723 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153723 [details]
Patch

Attachment 153723 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13312903

New failing tests:
transitions/equivalent-background-image-no-transition.html

[WebKit Review Bot]

Created attachment 153730 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 153723 [details]
Patch

Attachment 153723 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13316884

New failing tests:
transitions/equivalent-background-image-no-transition.html

[WebKit Review Bot]

Created attachment 153731 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 153735 [details]
Patch

Created attachment 153934 [details]
Patch

[noel gordon]

Ok cq+ given Simon's r+.

[WebKit Review Bot]

Comment on attachment 153934 [details]
Patch

Clearing flags on attachment: 153934

Committed r123426: <http://trac.webkit.org/changeset/123426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.