Bug 114313 – Web Inspector: CSS parser errors in the console should include column numbers

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Joseph Pecoraro]

In order to jump to the correct place in source code that is potentially pretty printed, the error should include a line number and column number. Currently CSSParser::logError only includes the line number and doesn't include a column number.

[Radar WebKit Bug Importer]

<rdar://problem/13614327>

[Joseph Pecoraro]

I'm looking at this right now because inline CSS warnings are always useless on minified CSS. Ben pointed me in the right direction and this looks very easy!

[Joseph Pecoraro]

Created attachment 248393 [details]
[PATCH] Proposed Fix

[Benjamin Poulain]

Comment on attachment 248393 [details]
[PATCH] Proposed Fix

That seems correct to me, rs=me

[WebKit Commit Bot]

Comment on attachment 248393 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 248393

Committed r181367: <http://trac.webkit.org/changeset/181367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This seems to have cause 7 layout tests to crash:
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r181367%20(2681)/results.html

[Chris Dumez]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010d51539a WTFCrash + 42
1   com.apple.WebCore             	0x000000010efa34ea WebCore::CSSParser::currentLocation() + 170 (CSSParser.cpp:10495)
2   com.apple.WebCore             	0x000000010ef439d6 cssyyparse(WebCore::CSSParser*) + 66182 (.CSSGrammar.y:1856)
3   com.apple.WebCore             	0x000000010ef70f5a WebCore::CSSParser::parseSheet(WebCore::StyleSheetContents*, WTF::String const&, int, int, WTF::Vector<WTF::RefPtr<WebCore::CSSRuleSourceData>, 0ul, WTF::CrashOnOverflow>*, bool) + 1178 (CSSParser.cpp:451)
4   com.apple.WebCore             	0x000000011090c0eb WebCore::StyleSheetContents::parseStringAtLineAndColumn(WTF::String const&, int, int, bool) + 123 (StyleSheetContents.cpp:325)
5   com.apple.WebCore             	0x000000010f7fd477 WebCore::InlineStyleSheetOwner::createSheet(WebCore::Element&, WTF::String const&) + 1255 (InlineStyleSheetOwner.cpp:149)
6   com.apple.WebCore             	0x000000010f7fcd04 WebCore::InlineStyleSheetOwner::createSheetFromTextContents(WebCore::Element&) + 68 (InlineStyleSheetOwner.cpp:97)
7   com.apple.WebCore             	0x000000010f7fcf7b WebCore::InlineStyleSheetOwner::finishParsingChildren(WebCore::Element&) + 59 (InlineStyleSheetOwner.cpp:92)
8   com.apple.WebCore             	0x000000010f68e80b WebCore::HTMLStyleElement::finishParsingChildren() + 43 (HTMLStyleElement.cpp:91)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142581

[Joseph Pecoraro]

Created attachment 248441 [details]
[PATCH] Proposed Fix

This addresses the ASSERT seen in css3/filters/filter-animation-from-none-multi-hw.html.

We need to have the token's start line/column, at the time currentLocation() is called we may have parsed past a newline and incremented m_columnOffsetForLine. So stash the token's column at the time we stash the token's line.

[Darin Adler]

Comment on attachment 248441 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/css/CSSParser.cpp:337
> +    , m_tokenStartColumnNumber(0)
>      , m_lastSelectorLineNumber(0)
> +    , m_columnOffsetForLine(0)
> +    , m_sheetStartLineNumber(0)
> +    , m_sheetStartColumnNumber(0)

It’s better to initialize in the header; avoids a need to list everything twice. Especially appropriate when the initial value is simple and logical (just a 0) and not specific to any particular constructor.

> Source/WebCore/css/CSSParser.cpp:11605
> -            if (*currentCharacter<SrcCharacterType>() == '\n')
> +            if (*currentCharacter<SrcCharacterType>() == '\n') {
>                  ++m_lineNumber;
> -            ++currentCharacter<SrcCharacterType>();
> +                ++currentCharacter<SrcCharacterType>();
> +                m_columnOffsetForLine = currentCharacterOffset();
> +            } else
> +                ++currentCharacter<SrcCharacterType>();

Could we use currentCharacterOffset() + 1 here and avoid having to repeat the ++currentCharacter?

> Source/WebCore/css/CSSParser.cpp:11904
> +        break;

Why add this break?

> Source/WebCore/css/CSSParser.h:108
> +    void parseSheet(StyleSheetContents*, const String&, int startLineNumber, int startColumnNumber, RuleSourceDataList*, bool logErrors);

Should we use WTF::TextPosition here instead of separate integers?

> Source/WebCore/css/CSSParser.h:467
> +    inline unsigned currentCharacterOffset();

The inline keyword here has no effect. I suggest omitting it. We should remove it from those other lines too, later.

> Source/WebCore/css/CSSParser.h:652
> +    void logError(const String& message, int lineNumber, int columnNumber);

Should we use WTF::TextPosition here instead of separate integers?

> Source/WebCore/css/StyleSheetContents.h:65
> +    bool parseStringAtLineAndColumn(const String&, int startLineNumber, int startColumnNumber, bool createdByParser);

Should we use WTF::TextPosition here instead of separate integers?

[Joseph Pecoraro]

(In reply to comment #11)
> Comment on attachment 248441 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248441&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:337
> > +    , m_tokenStartColumnNumber(0)
> >      , m_lastSelectorLineNumber(0)
> > +    , m_columnOffsetForLine(0)
> > +    , m_sheetStartLineNumber(0)
> > +    , m_sheetStartColumnNumber(0)
> 
> It’s better to initialize in the header; avoids a need to list everything
> twice. Especially appropriate when the initial value is simple and logical
> (just a 0) and not specific to any particular constructor.

Yes, I'm just caught in the normal position of:

  - modernize the entire class at the same time           => ugly diff, clean file
  - modernize just my piece and leave the rest unmodified => clean diff, ugly file
  - match the existing style of the file with new changes => clean diff, clean file


> > Source/WebCore/css/CSSParser.cpp:11605
> > -            if (*currentCharacter<SrcCharacterType>() == '\n')
> > +            if (*currentCharacter<SrcCharacterType>() == '\n') {
> >                  ++m_lineNumber;
> > -            ++currentCharacter<SrcCharacterType>();
> > +                ++currentCharacter<SrcCharacterType>();
> > +                m_columnOffsetForLine = currentCharacterOffset();
> > +            } else
> > +                ++currentCharacter<SrcCharacterType>();
> 
> Could we use currentCharacterOffset() + 1 here and avoid having to repeat
> the ++currentCharacter?

I originally had that and ran into issues. However I changed a lot since then so I'll retry and go with that approach if it works.


> > Source/WebCore/css/CSSParser.cpp:11904
> > +        break;
> 
> Why add this break?

Though not a specific WebKit style, the majority of our switches include a break in the default case. It is not required.


I agree with the other comments, and I'll address them.

[Joseph Pecoraro]

https://trac.webkit.org/r181426

[Joseph Pecoraro]

Follow-up fix for ASSERT seen in LayoutTests/printing/page-format-data.html.
https://trac.webkit.org/r181434