Summary: | Remove stylesheet pointer from StylePropertySet | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | CSS | Assignee: | 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
Antti Koivisto
2012-02-21 01:52:11 PST
Created attachment 127945 [details]
patch
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 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 http://trac.webkit.org/changeset/108345 - Renamed to addPropertyToAttributeStyle as suggested - Fixed handling of null context stylesheet (fixes the test crash too) 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 (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. 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)
(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) Created attachment 127969 [details]
Patch
Comment on attachment 127969 [details]
Patch
r=me, I'll land it manually.
Comment on attachment 127969 [details] Patch Clearing flags on attachment: 127969 Committed r108352: <http://trac.webkit.org/changeset/108352> All reviewed patches have been landed. Closing bug. |