Bug 22357 - Crash when setting className via SVG className.baseVal
Summary: Crash when setting className via SVG className.baseVal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2008-11-19 09:25 PST by Dmitry Stadnik
Modified: 2010-07-07 05:44 PDT (History)
3 users (show)

See Also:


Attachments
Test Page (887 bytes, application/xhtml+xml)
2008-11-19 09:26 PST, Dmitry Stadnik
no flags Details
Potential patch for issue 22357 (3.97 KB, patch)
2008-12-10 17:39 PST, Glenn Wilson
darin: review-
Details | Formatted Diff | Diff
Possible patch for issue 22357 (3.96 KB, patch)
2008-12-11 17:33 PST, Glenn Wilson
no flags Details | Formatted Diff | Diff
Possible patch to issue 22357 (3.97 KB, patch)
2008-12-22 12:14 PST, Glenn Wilson
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Stadnik 2008-11-19 09:25:15 PST
I call e.className.baseVal = "bum" where e is SVG ellipse element and browser crashes.
Comment 1 Dmitry Stadnik 2008-11-19 09:26:06 PST
Created attachment 25276 [details]
Test Page
Comment 2 Dmitry Stadnik 2008-11-19 09:35:55 PST
May be related to https://bugs.webkit.org/show_bug.cgi?id=20651
Comment 3 Dmitry Stadnik 2008-11-20 03:22:35 PST
Process:         Safari [3694]
Path:            /Applications/WebKit.app/Contents/MacOS/WebKit
Identifier:      org.webkit.nightly.WebKit
Version:         r38592 (38592)
Code Type:       X86 (Native)
Parent Process:  launchd [188]

Date/Time:       2008-11-19 18:28:01.882 +0100
OS Version:      Mac OS X 10.5.5 (9F33)
Report Version:  6

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x000000000000001c
Crashed Thread:  0

Thread 0 Crashed:
0   com.apple.WebCore             	0x00ddcd64 WebCore::CSSStyleSelector::matchRules(WebCore::CSSRuleSet*, int&, int&) + 100
1   com.apple.WebCore             	0x00ddd054 WebCore::CSSStyleSelector::matchUARules(int&, int&) + 84
2   com.apple.WebCore             	0x00df94f9 WebCore::CSSStyleSelector::styleForElement(WebCore::Element*, WebCore::RenderStyle*, bool, bool) + 345
3   com.apple.WebCore             	0x00f2b789 WebCore::Element::recalcStyle(WebCore::Node::StyleChange) + 313
4   com.apple.WebCore             	0x00f2b8e2 WebCore::Element::recalcStyle(WebCore::Node::StyleChange) + 658
5   com.apple.WebCore             	0x00f2b8e2 WebCore::Element::recalcStyle(WebCore::Node::StyleChange) + 658
6   com.apple.WebCore             	0x00f2b8e2 WebCore::Element::recalcStyle(WebCore::Node::StyleChange) + 658
7   com.apple.WebCore             	0x00f2b8e2 WebCore::Element::recalcStyle(WebCore::Node::StyleChange) + 658
8   com.apple.WebCore             	0x00f00892 WebCore::Document::recalcStyle(WebCore::Node::StyleChange) + 162
9   com.apple.WebCore             	0x00ef311f WebCore::Document::updateRendering() + 79
10  com.apple.WebCore             	0x00ef36c6 WebCore::Document::updateDocumentsRendering() + 86
11  com.apple.WebCore             	0x0135b661 WebCore::JSAbstractEventListener::handleEvent(WebCore::Event*, bool) + 1201
12  com.apple.WebCore             	0x00ef345d WebCore::Document::handleWindowEvent(WebCore::Event*, bool) + 173
13  com.apple.WebCore             	0x00f3b3a9 WebCore::EventTargetNode::dispatchWindowEvent(WTF::PassRefPtr<WebCore::Event>) + 121
14  com.apple.WebCore             	0x00f3d2a7 WebCore::EventTargetNode::dispatchWindowEvent(WebCore::AtomicString const&, bool, bool) + 103
15  com.apple.WebCore             	0x00efccf8 WebCore::Document::implicitClose() + 312
16  com.apple.WebCore             	0x00f78da9 WebCore::FrameLoader::checkCompleted() + 169
17  com.apple.WebCore             	0x00f7a740 WebCore::FrameLoader::finishedParsing() + 64
18  com.apple.WebCore             	0x00ef62bd WebCore::Document::finishedParsing() + 173
19  com.apple.WebCore             	0x00f7c7d8 WebCore::FrameLoader::endIfNotLoadingMainResource() + 120
20  com.apple.WebCore             	0x00f70282 WebCore::FrameLoader::finishedLoading() + 50
21  com.apple.WebCore             	0x011921cc WebCore::MainResourceLoader::didFinishLoading() + 44
22  com.apple.Foundation          	0x93d61097 -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] + 87
23  com.apple.Foundation          	0x93d61003 _NSURLConnectionDidFinishLoading + 147
24  com.apple.CFNetwork           	0x95f0f209 sendDidFinishLoadingCallback + 148
25  com.apple.CFNetwork           	0x95f0c180 _CFURLConnectionSendCallbacks + 1759
26  com.apple.CFNetwork           	0x95f0ba25 muxerSourcePerform + 283
27  com.apple.CoreFoundation      	0x91350615 CFRunLoopRunSpecific + 3141
28  com.apple.CoreFoundation      	0x91350cf8 CFRunLoopRunInMode + 88
29  com.apple.HIToolbox           	0x93841480 RunCurrentEventLoopInMode + 283
30  com.apple.HIToolbox           	0x93841299 ReceiveNextEventCommon + 374
31  com.apple.HIToolbox           	0x9384110d BlockUntilNextEventMatchingListInMode + 106
32  com.apple.AppKit              	0x9609a3ed _DPSNextEvent + 657
33  com.apple.AppKit              	0x96099ca0 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
34  com.apple.Safari              	0x000080be 0x1000 + 28862
35  com.apple.AppKit              	0x96092cdb -[NSApplication run] + 795
36  com.apple.AppKit              	0x9605ff14 NSApplicationMain + 574
37  com.apple.Safari              	0x000b9b46 0x1000 + 756550
Comment 4 Alexey Proskuryakov 2008-11-20 05:42:36 PST
Confirmed with r38590.
Comment 5 Glenn Wilson 2008-12-10 17:39:42 PST
Created attachment 25932 [details]
Potential patch for issue 22357

Here is a potential patch for this issue.

So it seems like this problem would arise when a css class is applied to an element that has no mappedAttributes.

From what I can tell, the issue arises in a difference in assertions between line 640 of CSSStyleSelector and StyledElement::classNames().  Specifically, when looking for styles to apply, CSSStyleSelector::matchRules checks that this element has a class applied (hasClass) without checking that it has mapped attributes set.  Typically, this isn't a problem, because the class attribute is a mapped attribute itself!

But on line 642, when StyledElement::classNames() is called, the classNames() method asserts that this element not only has a class set, but also has mappedAttributes set.  This will fail (and crash) for elements that have the class attribute set but no mappedAttributes.  In this repro case, it's an svg ellipse created on the fly with Javascript.

So, to fix this, I added one additional check on line 640 that does not enter this block if mappedAttributes is not set.  In essence, this changes CSSStyleSelector::matchRules to check on the same values that StyledElement::classNames does.

(Since m_styledElement is ASSERT'ed on the next line, it must be valid anyway, so the call is being done early.  If anything, this makes the ASSERT on 641 redundant.  Let me know if this should be removed.)
Comment 6 Darin Adler 2008-12-10 17:54:02 PST
Comment on attachment 25932 [details]
Potential patch for issue 22357

The class attribute itself is supposed to be a mapped attribute. It's not OK that we can't get class names -- that means that CSS styles that are based on class won't work! If hasClass is true, but there's no mapped attributes pointer, then something strange is going on.

Looking at the test case it looks like the key here is that the class name is being set by SVG "baseValue" code. I think that means that the SVG code is not setting the attribute properly, and that's resulting in the object being in a strange state. It's that animation code incorrectly setting the attribute that needs to be fixed, not the CSS selector matching code.

If the change was correct and was needed in this function, then it would also be needed in CSSStyleSelector::SelectorChecker::checkOneSelector.
Comment 7 Darin Adler 2008-12-10 17:55:30 PST
(In reply to comment #2)
> May be related to https://bugs.webkit.org/show_bug.cgi?id=20651

Definitely related.
Comment 8 Glenn Wilson 2008-12-11 11:27:41 PST
(In reply to comment #7)
> (In reply to comment #2)
> > May be related to https://bugs.webkit.org/show_bug.cgi?id=20651
> 
> Definitely related.
> 

There's one particular behavior in StyledElement::classAttributeChanged (which was once part of parseMappedAttribute) -- it sets that it has a class first (line 222), but doesn't actually set any mapped values if namedAttrMap doesn't already exist.

(Looking in SVGEllipseElement, setting cX, cY, etc. values on baseValue doesn't create this namedAttrMap, so that's probably why it doesn't exist.)

So, I think there are a few options here:
- Not set hasClass if namedAttrMap isn't set.  This doesn't seem like the right approach because we *should* be setting both.
- Create namedAttrMap if it doesn't already exist.  This could be done via createAttributeMap().  This seems like the right way, but I don't know all of the background of namedAttrMap to know.

What's the best way to do this?

Comment 9 Glenn Wilson 2008-12-11 17:33:10 PST
Created attachment 25970 [details]
Possible patch for issue 22357

From what I could find, StyledElement::classAttributeChanged method would set hasClass before checking for the existence of namedAttributeMap.  So it was possible to change the class attribute (via baseVal) without the namedAttributeMap ever getting created.  Later on, when CSSStyleSelector tries to figure out which styles to apply, it finds that StyledElement says it has a class set, but it has no mapped attributes!  Barf.

This patch modifies StyledElement::classAttributeChanged to create the namedAttributeMap if it doesn't exist, rather than just checking for its existence.  After all, if a StyledElement has its class attribute set, and the class is *always* a mapped attribute, shouldn't it be guaranteed that namedAttributeMap exists and is populated correctly?

I'm not sure if this is the right class to do this, since I don't know the entire life cycle of namedAttributeMap.  Should SVG elements automatically create namedAttributeMap when they are created?  Is there a better place to add this behavior other than StyledElement?
Comment 10 Eric Seidel (no email) 2008-12-15 13:55:35 PST
Comment on attachment 25970 [details]
Possible patch for issue 22357

The only question I have left is if namedAttrMap is ever supposed to be cleared?  Or is it such that once an element ever gets a single mapped attribute, it has a map for all time?  (I would assume the later, but I'm not sure).

If namedAttrMap is ever supposed to clear, then we should only be creating in the "set" case, not the "clear" case.  Otherwise looks fine.
Comment 11 Glenn Wilson 2008-12-22 12:14:29 PST
Created attachment 26211 [details]
Possible patch to issue 22357

Ah, good catch.  The previous patch would create the namedAttrMap regardless, which doesn't seem correct.  It should only create the namedAttrMap if it doesn't exist and it's not being cleared.

This new patch only creates namedAttrMap if it exists and the class name is being set, not cleared.
Comment 12 Darin Adler 2008-12-22 13:00:38 PST
Comment on attachment 26211 [details]
Possible patch to issue 22357

=> -    if (namedAttrMap) {
> -        if (i < length)
> -            mappedAttributes()->setClass(newClassString);
> -        else
> -            mappedAttributes()->clearClass();
> -    }
> +    if (i < length) {
> +        if (!namedAttrMap)
> +            createAttributeMap();
> +        mappedAttributes()->setClass(newClassString);
> +    } else
> +        mappedAttributes()->clearClass();

It's not correct to create an attribute map here. The map is supposed to be created on demand in functions like Element::attributes, and a map should not be created just because the class attribute was set on an element.

I need more information about when we're going to need the attribute map later, and we should consider putting the call to create it at that site instead of here. Or maybe when it's created the class isn't set up correctly in it? I need more info about how the test case is failing to understand what the right fix is.

This patch also introduces a null-dereferencing crash: The clearClass could dereference 0 if namedAttrMap is 0.
Comment 13 Rob Buis 2010-07-06 05:51:58 PDT
The crash seems gone, I think r62514 fixed it, can anyone confirm?
Cheers,

Rob.
Comment 14 Rob Buis 2010-07-07 05:44:02 PDT
As confirmed by krit, this one is fixed.