Bug 22357

Created attachment 25276 [details] Test Page

May be related to https://bugs.webkit.org/show_bug.cgi?id=20651

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

Confirmed with r38590.

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 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.

(In reply to comment #2) > May be related to https://bugs.webkit.org/show_bug.cgi?id=20651 Definitely related.

(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?

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 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.

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 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.

The crash seems gone, I think r62514 fixed it, can anyone confirm? Cheers, Rob.

As confirmed by krit, this one is fixed.