Closed
Bug 731813
Opened 13 years ago
Closed 13 years ago
Crash [@ nsAttrValue::ToString] listening for DOMAttrModified and removing a missing attribute
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: jruderman, Assigned: heycam)
References
Details
(Keywords: crash, testcase)
Crash Data
Attachments
(3 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
I think we should just check that the nsAttrValue* is not null inside nsSVGElement::MaybeSerializeAttrBeforeRemoval.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #601837 -
Flags: review?(jwatt)
Updated•13 years ago
|
Crash Signature: [@ nsAttrValue::ToString]
[@ nsAttrValue::ToString(nsAString_internal&)]
OS: Mac OS X → All
Hardware: x86_64 → All
Comment 4•13 years ago
|
||
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+
Comment 5•13 years ago
|
||
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
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
If you feel like adding any of that knowledge to the source in the form of comments, Cameron, that would be great. :)
Assignee | ||
Comment 9•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #601837 -
Attachment is obsolete: true
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Sure thing. https://hg.mozilla.org/integration/mozilla-inbound/rev/a19f6fd4f6f2
Flags: in-testsuite+
Target Milestone: --- → mozilla13
Comment 13•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a19f6fd4f6f2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•