Bug 11197 – REGRESSION: Specifying a counter for a CODE tag's content style property on before or after causes a crash.

• Component: CSS
• Status: RESOLVED FIXED

[Adam Elliot]

If you specify any counter, { content: counter(dummy,disc); }, on a code tag seems to crash the browser.  The crash does not occur on other tags.  Stable Safari does not crash, but the counters do not show up either.

[Adam Elliot]

Created attachment 10955 [details]
Reduction

Thread 0 Crashed:
0   com.apple.WebCore        	0x01e0c9bc WebCore::StringImpl::length() const + 20 (StringImpl.h:74)
1   com.apple.WebCore        	0x01b10dbc WebCore::RenderText::allAscii() const + 228 (RenderText.cpp:446)
2   com.apple.WebCore        	0x01b10e5c WebCore::RenderText::shouldUseMonospaceCache(WebCore::Font const*) const + 72 (RenderText.cpp:459)
3   com.apple.WebCore        	0x01b10f60 WebCore::RenderText::cacheWidths() + 56 (RenderText.cpp:468)
4   com.apple.WebCore        	0x01b13a08 WebCore::RenderText::setStyle(WebCore::RenderStyle*) + 532 (RenderText.cpp:128)
5   com.apple.WebCore        	0x01acecb4 WebCore::RenderContainer::updatePseudoChildForObject(WebCore::RenderStyle::PseudoId, WebCore::RenderObject*) + 2460 (RenderContainer.cpp:356)
6   com.apple.WebCore        	0x01acede8 WebCore::RenderContainer::updatePseudoChild(WebCore::RenderStyle::PseudoId) + 168 (RenderContainer.cpp:250)
7   com.apple.WebCore        	0x01adea84 WebCore::RenderInline::setStyle(WebCore::RenderStyle*) + 356 (RenderInline.cpp:68)
8   com.apple.WebCore        	0x01bdff1c WebCore::Node::createRendererIfNeeded() + 684 (Node.cpp:902)
9   com.apple.WebCore        	0x01be787c WebCore::Element::attach() + 44 (Element.cpp:560)
10  com.apple.WebCore        	0x018f46a0 WebCore::HTMLParser::insertNode(WebCore::Node*, bool) + 680 (HTMLParser.cpp:288)
11  com.apple.WebCore        	0x018f616c WebCore::HTMLParser::parseToken(WebCore::Token*) + 1388 (HTMLParser.cpp:220)
12  com.apple.WebCore        	0x018f9cb4 WebCore::HTMLTokenizer::processToken() + 624 (HTMLTokenizer.cpp:1645)
13  com.apple.WebCore        	0x018fd87c WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) + 7252 (HTMLTokenizer.cpp:1218)
14  com.apple.WebCore        	0x018fe384 WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) + 1444 (HTMLTokenizer.cpp:1444)
15  com.apple.WebCore        	0x019fe9fc WebCore::Frame::write(char const*, int) + 1200 (Frame.cpp:717)
16  com.apple.WebCore        	0x019f3014 WebCore::Frame::addData(char const*, int) + 340 (Frame.cpp:2787)
17  com.apple.WebCore        	0x01a3f1e8 -[WebCoreFrameBridge addData:] + 224 (WebCoreFrameBridge.mm:587)
18  com.apple.WebKit         	0x003351ac -[WebFrameBridge receivedData:textEncodingName:] + 276 (WebFrameBridge.m:502)
19  com.apple.WebKit         	0x0035365c -[WebHTMLRepresentation receivedData:withDataSource:] + 268 (WebHTMLRepresentation.m:150)
20  com.apple.WebKit         	0x0033f28c -[WebDataSource(WebInternal) _receivedData:] + 108 (WebDataSource.m:171)
21  com.apple.WebKit         	0x003d7878 -[WebFrameLoader committedLoadWithDocumentLoadState:data:] + 120 (WebFrameLoader.m:987)
22  com.apple.WebKit         	0x003e4704 -[WebDocumentLoadState commitLoadWithData:] + 140 (WebDocumentLoadState.m:299)
23  com.apple.WebKit         	0x003e489c -[WebDocumentLoadState receivedData:] + 168 (WebDocumentLoadState.m:313)
24  com.apple.WebKit         	0x003d4da0 -[WebFrameLoader _receivedData:] + 104 (WebFrameLoader.m:561)
25  com.apple.WebKit         	0x003dd2d0 -[WebMainResourceLoader addData:allAtOnce:] + 156 (WebMainResourceLoader.m:152)
26  com.apple.WebKit         	0x003db858 -[WebLoader didReceiveData:lengthReceived:allAtOnce:] + 128 (WebLoader.m:366)
27  com.apple.WebKit         	0x003de410 -[WebMainResourceLoader didReceiveData:lengthReceived:allAtOnce:] + 492 (WebMainResourceLoader.m:351)
28  com.apple.WebKit         	0x003dc1a4 -[WebLoader connection:didReceiveData:lengthReceived:] + 192 (WebLoader.m:466)
29  com.apple.Foundation     	0x929935d4 -[NSURLConnection(NSURLConnectionInternal) _sendDidReceiveDataCallback] + 564
30  com.apple.Foundation     	0x92991a74 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 488
31  com.apple.Foundation     	0x92991810 _sendCallbacks + 156
32  com.apple.CoreFoundation 	0x907dd4cc __CFRunLoopDoSources0 + 384
33  com.apple.CoreFoundation 	0x907dc9fc __CFRunLoopRun + 452
34  com.apple.CoreFoundation 	0x907dc47c CFRunLoopRunSpecific + 268
35  com.apple.HIToolbox      	0x93208740 RunCurrentEventLoopInMode + 264
36  com.apple.HIToolbox      	0x93207dd4 ReceiveNextEventCommon + 380
37  com.apple.HIToolbox      	0x93207c40 BlockUntilNextEventMatchingListInMode + 96
38  com.apple.AppKit         	0x9370bae4 _DPSNextEvent + 384
39  com.apple.AppKit         	0x9370b7a8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
40  com.apple.Safari         	0x00006740 0x1000 + 22336
41  com.apple.AppKit         	0x93707cec -[NSApplication run] + 472
42  com.apple.AppKit         	0x937f887c NSApplicationMain + 452
43  com.apple.Safari         	0x0005c77c 0x1000 + 374652
44  com.apple.Safari         	0x0005c624 0x1000 + 374308

[Stephanie Lewis]

4826798

[Andrew Wellington]

Created attachment 12451 [details]
Proposed patch

A simple patch to resolve this issue.

Another possible point for this patch would be not calling cacheWidths() from RenderText::setStyle if setText was not called. This is probably not a good spot however as this is not the only place setText may be called to change m_str's value.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Created an attachment (id=12451) [edit]
> Proposed patch
> 
> A simple patch to resolve this issue.

FWIW, this probably doesn't need to be a pixel test since it's testing non-crashing behavior.  :)

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> FWIW, this probably doesn't need to be a pixel test since it's testing
> non-crashing behavior.  :)

Unless you're also testing the before/after rendering for <code> elements that's not included in other tests.

[Darin Adler]

Comment on attachment 12451 [details]
Proposed patch

I believe a better fix is to fix allAscii() to work when m_str is 0. Just putting the for loop inside an if (m_str) would do the trick.

But this patch is also OK as-is.

[Darin Adler]

My patch removed the call to cacheWidths that was done after changing the text and moved it to setStyle instead. That's a problem since the value of shouldUseMonospaceCache is based on the value of allAscii. So I think we have a bug now when some all-ASCII text is replaced with text that has some non-ASCII characters in it. The monospace cache will be used in cases where it's not appropriate.

Seems like we need to move the cacheWidths call back into setInternalString.

[Darin Adler]

Created attachment 12590 [details]
much larger patch, passes layout tests, I think we should land this one

[Darin Adler]

Comment on attachment 12451 [details]
Proposed patch

Clearing the reviewed flag on this patch for now. Depending on feedback about my patch, might land this or mine.

Comment on attachment 12590 [details]
much larger patch, passes layout tests, I think we should land this one

\ No newline at end of file

Please add a newline.

+                if (style()->collapseWhiteSpace() && next && !next->isBR() && next->isText() && static_cast<RenderText*>(next)->textLength() > 0) {

The > 0 is unnecessary.

+                    if (nextText->textLength() != 0) {

Should be 'if (nextText->textLength())'.

+++ WebKitTools/Scripts/run-webkit-tests	(working copy)

Doesn't belong in this patch.

r=me w/o the run-webkit-tests changes.

Created attachment 12603 [details]
Test case for the monospace cache regression

[Darin Adler]

Committed revision 19027.