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

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: heycam)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Trunk
mozilla13
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 601781 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

6 years ago
Created attachment 601782 [details]
stack trace
I think we should just check that the nsAttrValue* is not null inside nsSVGElement::MaybeSerializeAttrBeforeRemoval.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Created attachment 601837 [details] [diff] [review]
Do not attempt to serialize SVG attributes for mutation events if they do not exist.
Attachment #601837 - Flags: review?(jwatt)

Updated

6 years ago
Crash Signature: [@ nsAttrValue::ToString] [@ nsAttrValue::ToString(nsAString_internal&)]
OS: Mac OS X → All
Hardware: x86_64 → All
Blocks: 629200
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.
Attachment #601837 - Flags: review?(jwatt) → review+
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
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.
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.
If you feel like adding any of that knowledge to the source in the form of comments, Cameron, that would be great. :)
Created attachment 602772 [details] [diff] [review]
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.
Attachment #602772 - Flags: review?(jwatt)
Attachment #602772 - Flags: review?(bzbarsky)
Attachment #601837 - Attachment is obsolete: true
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
Attachment #602772 - Flags: review?(bzbarsky) → review+
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");
Attachment #602772 - Flags: review?(jwatt) → review+
Sure thing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a19f6fd4f6f2
Flags: in-testsuite+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/a19f6fd4f6f2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.