Bug 40857 - Altering the CSS class of an attached SVG element causes WebKit to crash
Summary: Altering the CSS class of an attached SVG element causes WebKit to crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-18 14:00 PDT by Fady Samuel
Modified: 2010-07-05 19:03 PDT (History)
2 users (show)

See Also:


Attachments
Crash bug (1011 bytes, text/html)
2010-06-21 07:46 PDT, Fady Samuel
no flags Details
Possible fix (7.82 KB, patch)
2010-06-23 12:37 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Updated Patch (5.54 KB, patch)
2010-07-05 08:31 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Updated Patch -- fixed junk in changelog (5.54 KB, patch)
2010-07-05 08:35 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Updated Patch -- The previous one was the wrong file (4.04 KB, patch)
2010-07-05 08:36 PDT, Fady Samuel
darin: review-
Details | Formatted Diff | Diff
Updated Patch with suggested change (4.01 KB, patch)
2010-07-05 10:20 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2010-06-18 14:00:02 PDT
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.
Comment 1 Fady Samuel 2010-06-21 07:46:04 PDT
Created attachment 59249 [details]
Crash bug
Comment 2 Fady Samuel 2010-06-23 11:28:32 PDT
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.
Comment 3 Fady Samuel 2010-06-23 12:37:36 PDT
Created attachment 59551 [details]
Possible fix
Comment 4 Alexey Proskuryakov 2010-06-23 16:59:06 PDT
Thanks for tackling this! I'm not trying to do a full review, but I do have a few comments:

+2010-06-23    <>

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.
Comment 5 Fady Samuel 2010-07-05 08:31:25 PDT
Created attachment 60540 [details]
Updated Patch
Comment 6 Fady Samuel 2010-07-05 08:35:27 PDT
Created attachment 60541 [details]
Updated Patch -- fixed junk in changelog
Comment 7 Fady Samuel 2010-07-05 08:36:55 PDT
Created attachment 60542 [details]
Updated Patch -- The previous one was the wrong file
Comment 8 Darin Adler 2010-07-05 09:38:44 PDT
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.
Comment 9 Fady Samuel 2010-07-05 10:20:38 PDT
Created attachment 60553 [details]
Updated Patch with suggested change
Comment 10 WebKit Commit Bot 2010-07-05 19:03:40 PDT
Comment on attachment 60553 [details]
Updated Patch with suggested change

Clearing flags on attachment: 60553

Committed r62514: <http://trac.webkit.org/changeset/62514>
Comment 11 WebKit Commit Bot 2010-07-05 19:03:45 PDT
All reviewed patches have been landed.  Closing bug.