Bug 20855 - Crashes when <style> @import "style.css"; </style> importing a CSS file, but works fine when same CSS is inlined
Summary: Crashes when <style> @import "style.css"; </style> importing a CSS file, but ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Chris Marrin
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-09-15 07:11 PDT by John Engelhart
Modified: 2008-09-29 14:09 PDT (History)
4 users (show)

See Also:


Attachments
index.html file (527 bytes, text/html)
2008-09-15 07:21 PDT, John Engelhart
no flags Details
rk.css - the CSS file for index.html (2.93 KB, text/css)
2008-09-15 07:22 PDT, John Engelhart
no flags Details
Crash reporter file w/ stack trace (29.11 KB, text/plain)
2008-09-15 07:35 PDT, John Engelhart
no flags Details
Patch, including LayoutTest file (7.55 KB, patch)
2008-09-18 11:06 PDT, Chris Marrin
hyatt: review-
Details | Formatted Diff | Diff
Test case (5.09 KB, patch)
2008-09-22 13:10 PDT, Chris Marrin
darin: review-
Details | Formatted Diff | Diff
New patch with testcase and changelog entry (5.77 KB, patch)
2008-09-22 14:48 PDT, Chris Marrin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Engelhart 2008-09-15 07:11:32 PDT
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...
Comment 1 John Engelhart 2008-09-15 07:21:04 PDT
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.
Comment 2 John Engelhart 2008-09-15 07:22:36 PDT
Created attachment 23438 [details]
rk.css - the CSS file for index.html

The corresponding .css file for the index.html attachment.
Comment 3 John Engelhart 2008-09-15 07:35:22 PDT
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
Comment 4 Matt Lilek 2008-09-15 09:00:12 PDT
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)
Comment 5 John Engelhart 2008-09-15 11:19:44 PDT
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.
Comment 6 John Engelhart 2008-09-15 16:02:51 PDT
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.. :)
Comment 7 Simon Fraser (smfr) 2008-09-17 16:52:30 PDT
At:
        m_style = new (doc->renderArena()) RenderStyle();
doc is null.
Comment 8 Chris Marrin 2008-09-18 11:02:32 PDT
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...
Comment 9 Chris Marrin 2008-09-18 11:06:44 PDT
Created attachment 23533 [details]
Patch, including LayoutTest file
Comment 10 Eric Seidel (no email) 2008-09-20 13:18:52 PDT
Comment on attachment 23533 [details]
Patch, including LayoutTest file

Hyatt really should look at this.
Comment 11 Dave Hyatt 2008-09-20 21:15:14 PDT
Comment on attachment 23533 [details]
Patch, including LayoutTest file

r-

Just use:

m_checker.m_document

instead of

sheet->doc()
Comment 12 Dave Hyatt 2008-09-20 21:17:25 PDT
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.

Comment 13 Dave Hyatt 2008-09-20 21:18:30 PDT
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.


Comment 14 Chris Marrin 2008-09-22 13:09:23 PDT
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
Comment 15 Chris Marrin 2008-09-22 13:10:17 PDT
Created attachment 23661 [details]
Test case
Comment 16 Darin Adler 2008-09-22 13:40:23 PDT
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+.
Comment 17 Chris Marrin 2008-09-22 14:48:54 PDT
Created attachment 23670 [details]
New patch with testcase and changelog entry
Comment 18 Darin Adler 2008-09-22 14:51:45 PDT
Comment on attachment 23670 [details]
New patch with testcase and changelog entry

r=me
Comment 19 Simon Fraser (smfr) 2008-09-29 14:09:39 PDT
	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