Bug 20855 – Crashes when <style> @import "style.css"; </style> importing a CSS file, but works fine when same CSS is inlined

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

[John Engelhart]

Hard crasher, every time.  I will attach the distilled .html and .css file after I submit this bug.

I'm not sure exactly which part of the CSS (or whatever) is causing the problem, but I managed to get it down to a ~500 byte html file and ~3000 byte CSS that reproduces the problem.  I've been using the new animation CSS stuff, and the chunk of CSS code was the last thing I added in before it started crashing (it was a copy/paste, worked on the CSS parts in a separate html file, then copied the finished results to the main .css style file, and boom).

In short, when the attached CSS stylesheet is @imported, it crashes.  Remove the @import.. and replace it with the text of that CSS file, and everything works fine...

[John Engelhart]

Created attachment 23437 [details]
index.html file

CSS file will be in the next attachment.

FYI, the nightly I'm using is 35942.

I'm posting this bug with the latest Safari developer preview (Safari 4.0 5528.1), and when I went to attach the file, the quicklook daemon kept croaking... so I'm guessing that it's probably not limited to just the nightly.

[John Engelhart]

Created attachment 23438 [details]
rk.css - the CSS file for index.html

The corresponding .css file for the index.html attachment.

[John Engelhart]

Created attachment 23439 [details]
Crash reporter file w/ stack trace

One of the CrashReporter files w/ a stack trace and what not in it, for reference.

When attaching these files, the quicklook daemon kept crashing when attempting to render these buggy index+css files.  I took a quick peek at the quicklookd crash stack traceback, and it's crashing in the exact same place that this.

FYI, the quicklookd is not using this nightlies web* libs:

0x90a43000 - 0x90b13ffb  com.apple.WebKit 5528 (5528.1) <db458f83059996313edbeb7cd46bdbb8> /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit
0x9251f000 - 0x92d9affb  com.apple.WebCore 5528 (5528.1) <3d7bf9b795bec6314635e23f0e1210a6> /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/WebCore

[Matt Lilek]

Confirmed with r36444:

Thread 0 Crashed:
0   com.apple.WebCore             	0x032acb17 WebCore::Document::renderArena() + 9 (Document.h:387)
1   com.apple.WebCore             	0x032a6f70 WebCore::CSSStyleSelector::addKeyframeStyle(WebCore::Document*, WebCore::WebKitCSSKeyframesRule const*) + 386 (CSSStyleSelector.cpp:456)
2   com.apple.WebCore             	0x032a7634 WebCore::CSSRuleSet::addRulesFromSheet(WebCore::CSSStyleSheet*, WebCore::MediaQueryEvaluator const&, WebCore::CSSStyleSelector*) + 1130 (CSSStyleSelector.cpp:2447)
3   com.apple.WebCore             	0x032a734f WebCore::CSSRuleSet::addRulesFromSheet(WebCore::CSSStyleSheet*, WebCore::MediaQueryEvaluator const&, WebCore::CSSStyleSelector*) + 389 (CSSStyleSelector.cpp:2457)
4   com.apple.WebCore             	0x032a8f4a WebCore::CSSStyleSelector::CSSStyleSelector(WebCore::Document*, WebCore::String const&, WebCore::StyleSheetList*, WebCore::CSSStyleSheet*, bool, bool) + 1286 (CSSStyleSelector.cpp:432)
5   com.apple.WebCore             	0x033f1195 WebCore::Document::recalcStyleSelector() + 2703 (Document.cpp:2329)

[John Engelhart]

I don't know the webkit code base, but I took a very quick stab at trying to find the problem.

The problem seems to be in WebCore/css/CSSStyleSelector.cpp CSSStyleSelector::addKeyframeStyle().

Now, keep in mind I worked this out from taking the stack trace, a quick disassemble of the library (no debugging symbols), and did the sort of 'line up function calls / basic blocks' as best you can.

for (unsigned i = 0; i < rule->length(); ++i) {
  const WebKitCSSKeyframeRule* kf = rule->item(i);
  m_style = new (doc->renderArena()) RenderStyle();
  m_style->ref();
  CSSMutableStyleDeclaration* decl = kf->style();

If I had to guess, I'd say it's that last line.  Well, the first line in the loop sets the value, and it (seems) to set it to NULL in this case.

In WebCore/css/WebKitCSSKeyframesRule.cpp we have this:

WebKitCSSKeyframeRule* WebKitCSSKeyframesRule::item(unsigned index)
 {
     CSSRule* rule = m_lstCSSRules.get()->item(index);
     return (rule && rule->isKeyframeRule()) ? static_cast<WebKitCSSKeyframeRule*>(rule) : 0;
 }
 
My guess, not knowing the source base at all, is that this is kicking back a NULL because isKeyframeRule() is false.. ?

For me at least, trying to glean anything beyond that is in the "takes longer than 15 minutes" category.  I have no idea what the execution path is that builds up these rule structures in the first place, so I can't check if the problem starts way up stream.  Of course, anyone who knows the code base will probably take a look at this and figure out the problem and the fix in about 10 seconds.

[John Engelhart]

I managed to get this down to the minimal bits that cause the problem.  The absolute minimum .css file that causes the problem is something long the lines of:

@-webkit-keyframes 'pulse' { from { left: 5px; -webkit-animation-timing-function: ease-out; } }

(along with a .html file that has a <style> @import "file.css"; </style>, of course)

I think the -webkit-ani... is superfluous, it just happens to be in the .css file I was using to test.. and I really don't want to crash this browser I'm writing this message in to verify that it's not needed.. :)

[Simon Fraser (smfr)]

At:
        m_style = new (doc->renderArena()) RenderStyle();
doc is null.

[Chris Marrin]

When loading an @import sheet, there is no doc, causing the crash when creating the RenderStyle. But this is a temporary style, filled with decls, copied to another style and then destroyed. So I now just create and destroy it with the default new and delete. Patch with new testcases on the way...

[Chris Marrin]

Created attachment 23533 [details]
Patch, including LayoutTest file

[Eric Seidel (no email)]

Comment on attachment 23533 [details]
Patch, including LayoutTest file

Hyatt really should look at this.

[Dave Hyatt]

Comment on attachment 23533 [details]
Patch, including LayoutTest file

r-

Just use:

m_checker.m_document

instead of

sheet->doc()

[Dave Hyatt]

By the way, it's total crap that stylesheets have a doc() method that is null for @import sheets.  That's just dumb.  The code you wrote should have been fine.  We should add a bug to fix @import sheets to actually have valid doc pointers.

[Dave Hyatt]

In  the meantime though you can just switch over to m_checker.m_document to work around the @import problem without having to do global new/delete hacks.

[Chris Marrin]

Comment on attachment 23533 [details]
Patch, including LayoutTest file

Hyatt has fixed this bug in changelist 36771. I will submit the tests as a separate patch

[Chris Marrin]

Created attachment 23661 [details]
Test case

[Darin Adler]

Comment on attachment 23661 [details]
Test case

Looks great! But even LayoutTests changes need ChangeLog. Please upload another copy with the log entry and I'll do a review+.

[Chris Marrin]

Created attachment 23670 [details]
New patch with testcase and changelog entry

[Darin Adler]

Comment on attachment 23670 [details]
New patch with testcase and changelog entry

r=me

[Simon Fraser (smfr)]

	M	LayoutTests/ChangeLog
	A	LayoutTests/animations/import-crash-expected.txt
	A	LayoutTests/animations/import-crash.html
	A	LayoutTests/animations/import-expected.txt
	A	LayoutTests/animations/import.html
	A	LayoutTests/animations/resources/keyframes.css
Committed r37073