731813 - Crash [@ nsAttrValue::ToString] listening for DOMAttrModified and removing a missing attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Cameron McCormack (:heycam)]

I think we should just check that the nsAttrValue* is not null inside nsSVGElement::MaybeSerializeAttrBeforeRemoval.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Do not attempt to serialize SVG attributes for mutation events if they do not exist.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 601837 [details] [diff] [review]
Do not attempt to serialize SVG attributes for mutation events if they do not exist.

Review of attachment 601837 [details] [diff] [review]:
-----------------------------------------------------------------

We should have tests for this. Rather than adding a crash test (which I'm sure you know by now I hate), please add to test_dataTypesModEvents.html for the various types tested there, checking that removing an attribute that is not set does not dispatch a mutation event. That will check both that we don't crash, and that we behave correctly.

::: content/svg/content/src/nsSVGElement.cpp
@@ +1473,3 @@
>    nsAutoString serializedValue;
> +  attrValue->ToString(serializedValue);
> +  nsAttrValue newAttrValue(serializedValue);

This is the _old_ attribute value we're serializing here, so call this variable oldAttrValue.

Comment 5

[Jonathan Watt [:jwatt]]

Hmm, except that that test file doesn't test transforms. Probably you should add to the other test files that use MutationEventChecker too.

https://mxr.mozilla.org/mozilla-central/search?string=MutationEventChecker

Specifically for testing the transform attribute that mean also adding a check to test_SVGTransformList.xhtml

Comment 6

[Cameron McCormack (:heycam)]

Looking into modifying these tests, I find that removeAttribute("transform") and removeAttributeNS(null, "transform") behave differently.  The former doesn't cause a crash.  It seems that in nsGenericElement::RemoveAttributeNS it does not correctly check that the attribute exists before calling UnsetAttr, but nsGenericElement::RemoveAttribute does.  Boris, some questions:

* Should UnsetAttr ever be called if the attribute doesn't exist?

* Is it right that RemoveAttribute does this strong reference holding while RemoveAttribute doesn't?

* The `nsid == kNameSpaceID_Unknown` check and comment in RemoveAttributeNS don't seem to match; GetNameSpaceID() is returning kNameSpaceID_None when elt.removeAttributeNS(null, "transform") is called even when the attribute doesn't exist.

Comment 7

[Boris Zbarsky [:bzbarsky]]

RemoveAttribute "checks" just to canonicalize the name it has, basically.  If it could skip that, it would.

> * Should UnsetAttr ever be called if the attribute doesn't exist?

I don't see why it couldn't be.  It's public API in Gecko....  It's trivial to trigger such calls in general, btw; pick a boolean DOM property that reflects an attribute, and set it to false.  We'll call UnsetAttr whether the attribute is set or not.

> * Is it right that RemoveAttribute does this strong reference holding while
> RemoveAttribute doesn't?

I assume the second one was supposed to be RemoveAttributeNS?  In any case, both hold a strong ref to the atom they're passing to UnsetAttr, as they should.

> * The `nsid == kNameSpaceID_Unknown` check and comment in RemoveAttributeNS don't seem
> to match

If nsid == kNameSpaceID_Unknown, that means that there can't possibly be such an attribute, because we've never even encountered this namespace.  Furthermode, passing around kNameSpaceID_Unknown and pretending it's a real namespace is not allowed; hence the early return is needed, not just an optimization.

There can be plenty of cases when nsid is a known namespace but the attr is not set, of course.

Comment 8

[Jonathan Watt [:jwatt]]

If you feel like adding any of that knowledge to the source in the form of comments, Cameron, that would be great. :)

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Do not attempt to serialize SVG attributes for mutation events if they do not exist. (v1.1)

Thanks Boris, understand now.  r? for the nsGenericContent.cpp comment changes;
hope they're not oversharing.

r?jwatt for the test changes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 602772 [details] [diff] [review]
Do not attempt to serialize SVG attributes for mutation events if they do not exist. (v1.1)

r=me

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 602772 [details] [diff] [review]
Do not attempt to serialize SVG attributes for mutation events if they do not exist. (v1.1)

Review of attachment 602772 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/test/test_SVGLengthList.xhtml
@@ +63,5 @@
>    lengths[0].newValueSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_NUMBER, 10);
> +  // -- Attribute removal
> +  eventChecker.expect("remove");
> +  text.removeAttribute("x");
> +  text.removeAttributeNS(null, "x");

Can you make this:

  // -- Attribute removal
  eventChecker.expect("remove");
  text.removeAttribute("x");
  // -- Non-existent attribute removal
  eventChecker.expect("");
  text.removeAttribute("x");
  text.removeAttributeNS(null, "x");

::: content/svg/content/test/test_SVGNumberList.xhtml
@@ +52,5 @@
>    text.setAttribute("rotate", "17 20 30");
> +  // -- Attribute removal
> +  eventChecker.expect("remove");
> +  text.removeAttribute("rotate");
> +  text.removeAttributeNS(null, "rotate");

And this:

  // -- Attribute removal
  eventChecker.expect("remove");
  text.removeAttribute("rotate");
  // -- Non-existent attribute removal
  eventChecker.expect("");
  text.removeAttribute("rotate");
  text.removeAttributeNS(null, "rotate");

::: content/svg/content/test/test_SVGPathSegList.xhtml
@@ +108,5 @@
> +
> +  // -- Attribute removal
> +  eventChecker.expect("remove");
> +  path.removeAttribute("d");
> +  path.removeAttributeNS(null, "d");

And this:

  // -- Attribute removal
  eventChecker.expect("remove");
  text.removeAttribute("d");
  // -- Non-existent attribute removal
  eventChecker.expect("");
  text.removeAttribute("d");
  text.removeAttributeNS(null, "d");

::: content/svg/content/test/test_SVGPointList.xhtml
@@ +52,5 @@
>    polyline.setAttribute("points", "30,375 150,380");
> +  // -- Attribute removal
> +  eventChecker.expect("remove");
> +  polyline.removeAttribute("points");
> +  polyline.removeAttributeNS(null, "points");

And this:

  // -- Attribute removal
  eventChecker.expect("remove");
  text.removeAttribute("points");
  // -- Non-existent attribute removal
  eventChecker.expect("");
  text.removeAttribute("points");
  text.removeAttributeNS(null, "points");

::: content/svg/content/test/test_SVGTransformList.xhtml
@@ +427,5 @@
> +
> +  // removeAttributeNS
> +  eventChecker.expect("remove");
> +  g.removeAttributeNS(null, "transform");
> +  g.removeAttributeNS(null, "transform");

And this:

  // -- Attribute removal
  eventChecker.expect("remove");
  text.removeAttribute("transform");
  // -- Non-existent attribute removal
  eventChecker.expect("");
  text.removeAttribute("transform");
  text.removeAttributeNS(null, "transform");

Comment 12

[Cameron McCormack (:heycam)]

Sure thing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a19f6fd4f6f2

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a19f6fd4f6f2