Bug 40857 – Altering the CSS class of an attached SVG element causes WebKit to crash

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Fady Samuel]

The attached html file causes WebKit-based browsers to crash (Safari/Chrome). 

It will crash after you click ok in the javascript alert box.

I looked a bit into this:

The javascript line: 

ellipse.className.baseVal = "cls1";

results in a call to StyledElement::classNames which then requests the classNames from the attribute map. 

At the time, the attribute map does not exist. An assert fails because it has not yet been created. 

It seems a simple fix is to create it if it hasn't been created. 

Looking at other code, it appears that the attribute map is created lazily when needed. 

According to Darin Adler, it’s not legal to call StyledElement::classNames without first checking hasClass. And if hasClass is true, then there will already be an attribute map. Calling mappedAttributes() or attributeMap() instead of calling attributes(false) is done for speed.

I am currently investigating this further.

[Fady Samuel]

Created attachment 59249 [details]
Crash bug

[Fady Samuel]

I spent about a day trying to figure out what the underlying cause of the problem is. It seems like Darin Adler's statement that hasClass being true means the attribute map will already be there is false. If that's the expected behavior then a simple fix is to make sure the attribute map is created when set the hasClass flag is set to true. I will post a patch shortly. Please let me know if you do not feel this fixes the underlying problem.

[Fady Samuel]

Created attachment 59551 [details]
Possible fix

[Alexey Proskuryakov]

Thanks for tackling this! I'm not trying to do a full review, but I do have a few comments:

[phone]    <>

Please add your name and e-mail address.

+++ b/LayoutTests/platform/chromium-linux/svg/css/svg-ellipse-render-crash-expected.txt

Since this is a test for a crasher, it needn't be a pixel one. You can turn it into a text test by adding a script:
if (window.layoutTestController)
    layoutTestController.dumpAsText();

+    if (!namedAttrMap) {
+      attributes(false);
     }

Coding style errors: there should be no braces around single line code blocks, and code should be indented by 4 spaces.

[Fady Samuel]

Created attachment 60540 [details]
Updated Patch

[Fady Samuel]

Created attachment 60541 [details]
Updated Patch -- fixed junk in changelog

[Fady Samuel]

Created attachment 60542 [details]
Updated Patch -- The previous one was the wrong file

[Darin Adler]

Comment on attachment 60542 [details]
Updated Patch -- The previous one was the wrong file

You did not put this patch up for review, which is done by setting the review flag to "?", but I assume you want it reviewed.

> +    if (!namedAttrMap)
> +        attributes(false);
> +    if (hasClass)
> +        mappedAttributes()->setClass(newClassString);
> +    else
> +        mappedAttributes()->clearClass();

The attributes function is not meant to be called just for its side effect. If you have determined it needs to be called, then you should use its result and call setClass and clearClass on that. There is no need to use the mappedAttributes function.

Also, there is no need to change the clearClass code path. In that path, checking namedAttrMap for 0 seems the right way to go. It's only in the setClass path that we'd want to call attributes to force the map to be created. Something like this:

    if (hasClass)
        attributes()->setClass(newClassString);
    else {
        if (namedAttrMap)
            namedAttrMap->clearClass();
    }

That would work well, I think.

[Fady Samuel]

Created attachment 60553 [details]
Updated Patch with suggested change

[WebKit Commit Bot]

Comment on attachment 60553 [details]
Updated Patch with suggested change

Clearing flags on attachment: 60553

Committed r62514: <http://trac.webkit.org/changeset/62514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.