Bug 79092

Summary: Remove stylesheet pointer from StylePropertySet
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CSSAssignee: Alexander Færøy <ahf>
Status: RESOLVED FIXED    
Severity: Normal CC: ahf, dglazkov, kling, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77745    
Attachments:
Description Flags
patch
none
workaround for this build breakage
none
Patch none

Description Antti Koivisto 2012-02-21 01:52:11 PST
The context should be passed as parameter for CSS parser invoking setters that actually need it.
Comment 1 Antti Koivisto 2012-02-21 02:12:43 PST
Created attachment 127945 [details]
patch
Comment 2 Andreas Kling 2012-02-21 03:24:11 PST
Comment on attachment 127945 [details]
patch

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

r=me with some glorious exposition:

> Source/WebCore/css/CSSParser.cpp:349
> +    Document* document = contextStyleSheet->findDocument();

Are we sure it's safe to always dereference 'contextStyleSheet' here? An assertion might be nice.

> Source/WebCore/css/CSSParser.cpp:473
> +    Document* document = contextStyleSheet->findDocument();

Same story.

> Source/WebCore/css/WebKitCSSMatrix.cpp:56
> -    if (CSSParser::parseValue(styleDeclaration.get(), CSSPropertyWebkitTransform, string, true, true)) {
> +    if (CSSParser::parseValue(styleDeclaration.get(), CSSPropertyWebkitTransform, string, true, true, 0)) {

Here's a call site where parseValue() would have a null contextStyleSheet.
Though it won't be a problem for CSSPropertyWebkitTransform, it sets a bad precedent.

> Source/WebCore/dom/StyledElement.h:72
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, int value);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, double value, CSSPrimitiveValue::UnitTypes);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, const String& value);

This name is not entirely fabulous, as the function is only concerned with a single property at a time. addPropertyToAttributeStyle() perhaps?
Comment 3 WebKit Review Bot 2012-02-21 03:47:40 PST
Comment on attachment 127945 [details]
patch

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

New failing tests:
fast/dom/css-delete-doc.html
Comment 4 Antti Koivisto 2012-02-21 05:49:57 PST
http://trac.webkit.org/changeset/108345

- Renamed to addPropertyToAttributeStyle as suggested
- Fixed handling of null context stylesheet (fixes the test crash too)
Comment 5 Csaba Osztrogonác 2012-02-21 06:10:04 PST
Reopen, because it broke the Qt build:
cc1plus: warnings being treated as errors
../../../../Source/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseValue(WebCore::StylePropertySet*, int, const WTF::String&, bool, bool, WebCore::CSSStyleSheet*)':
../../../../Source/WebCore/css/CSSParser.cpp:472: error: 'value$m_ptr' may be used uninitialized in this function
../../../../Source/WebCore/css/CSSParser.cpp:472: note: 'value$m_ptr' was declared here
Comment 6 Antti Koivisto 2012-02-21 06:41:24 PST
(In reply to comment #5)
> Reopen, because it broke the Qt build:
> cc1plus: warnings being treated as errors
> ../../../../Source/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseValue(WebCore::StylePropertySet*, int, const WTF::String&, bool, bool, WebCore::CSSStyleSheet*)':
> ../../../../Source/WebCore/css/CSSParser.cpp:472: error: 'value$m_ptr' may be used uninitialized in this function
> ../../../../Source/WebCore/css/CSSParser.cpp:472: note: 'value$m_ptr' was declared here

That looks like a problem with the compiler. Not sure how to work around that. Maybe just different warning flags.
Comment 7 Csaba Osztrogonác 2012-02-21 06:49:33 PST
Created attachment 127961 [details]
workaround for this build breakage

I don't think if we should disable "-Wuninitialized" to fix only this situation.
This warning hits with gcc 4.4.5 (Debian Squeeze)
Comment 8 Csaba Osztrogonác 2012-02-21 07:26:39 PST
(In reply to comment #7)
> Created an attachment (id=127961) [details]
> workaround for this build breakage
> 
> I don't think if we should disable "-Wuninitialized" to fix only this situation.
> This warning hits with gcc 4.4.5 (Debian Squeeze)

Landed in http://trac.webkit.org/changeset/108351/trunk/Source/WebCore/css/CSSParser.cpp

But it seems there are more similar bugs with gcc 4.5.2 (Ubuntu 11.04)
Comment 9 Alexander Færøy 2012-02-21 07:26:57 PST
Created attachment 127969 [details]
Patch
Comment 10 Csaba Osztrogonác 2012-02-21 07:29:25 PST
Comment on attachment 127969 [details]
Patch

r=me, I'll land it manually.
Comment 11 Csaba Osztrogonác 2012-02-21 07:31:02 PST
Comment on attachment 127969 [details]
Patch

Clearing flags on attachment: 127969

Committed r108352: <http://trac.webkit.org/changeset/108352>
Comment 12 Csaba Osztrogonác 2012-02-21 07:31:11 PST
All reviewed patches have been landed.  Closing bug.