[webkit-reviews] review denied: [Bug 22357] Crash when setting className via SVG className.baseVal : [Attachment 26211] Possible patch to issue 22357

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Glenn Wilson <gwilson at google.com>'s
request for review:
Bug 22357: Crash when setting className via SVG className.baseVal
https://bugs.webkit.org/show_bug.cgi?id=22357

Attachment 26211: Possible patch to issue 22357
https://bugs.webkit.org/attachment.cgi?id=26211&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
=> -	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.