M-C of 15 Jan 2011, release build, -g -O2, --disable-jemalloc. TEST_PATH=content/svg/content/test/test_SVGxxxList.xhtml produces the error below (multiple times) From analysis of nsSMILAnimationFunction::ParseAttr PRBool nsSMILAnimationFunction::ParseAttr(nsIAtom* aAttName, const nsISMILAttr& aSMILAttr, nsSMILValue& aResult, PRBool& aPreventCachingOfSandwich) const { nsAutoString attValue; if (GetAttr(aAttName, attValue)) { PRBool preventCachingOfSandwich; nsresult rv = aSMILAttr.ValueFromString(attValue, mAnimationElement, aResult, preventCachingOfSandwich); if (NS_FAILED(rv)) return PR_FALSE; if (preventCachingOfSandwich) { <---------- XXXXXX HERE aPreventCachingOfSandwich = PR_TRUE; } } return PR_TRUE; } valgrind is complaining at the marked point above. Seems like aSMILAttr.ValueFromString(...) is returning a non-NS_FAILED value, but nevertheless doesn't set its 4th (ref) parameter to anything. I tried to look at all the possible places it could call and only found 2. Both of them look OK: ./nsSMILCSSProperty.cpp:nsSMILCSSProperty::ValueFromString looks ok ./nsSMILCSSValueType.cpp :nsSMILCSSValueType::ValueFromString doesn't return nsresult (wrong type) ./nsSMILMappedAttribute.cpp :nsSMILMappedAttribute::ValueFromString looks ok So am mystified. I suspect this is related to bug 616362, but not sure how. ----------------------------------------------------------------------- Conditional jump or move depends on uninitialised value(s) at 0x5E980D0: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr const&, nsSMILValue&, int&) const (content/smil/nsSMILAnimationFunction.cpp:738) by 0x5E995C9: nsSMILAnimationFunction::GetValues(nsISMILAttr const&, nsTArray<nsSMILValue, nsTArrayDefaultAllocator>&) (content/smil/nsSMILAnimationFunction.cpp:789) by 0x5E98FA0: nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&) (content/smil/nsSMILAnimationFunction.cpp:241) by 0x5E99E2A: nsSMILCompositor::ComposeAttribute() (content/smil/nsSMILCompositor.cpp:124) by 0x5E96018: DoComposeAttribute(nsSMILCompositor*, void*) (content/smil/nsSMILAnimationController.cpp:390) by 0x64475AA: PL_DHashTableEnumerate (ff-opt/xpcom/build/pldhash.c:754) by 0x5E96E38: nsSMILAnimationController::DoSample(int) (ff-opt/content/smil/../../dist/include/nsTHashtable.h:241) by 0x5E6C380: nsSVGSVGElement::SetCurrentTime(float) (content/svg/content/src/nsSVGSVGElement.cpp:555) by 0x64A3946: NS_InvokeByIndex_P (xpcom/reflect/xptcall/src/md /unix/xptcinvoke_x86_64_unix.cpp:208) by 0x5F45B57: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (js/src/xpconnect/src/xpcwrappednative.cpp:3066) by 0x5F4DB65: XPC_WN_CallMethod(JSContext*, unsigned int, unsigned long*) (js/src/xpconnect/src/xpcwrappednativejsops.cpp:1593) by 0x678E64C: CallCompiler::generateNativeStub() (js/src/jscntxtinlines.h:692) Uninitialised value was created by a stack allocation at 0x5E98000: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr const&, nsSMILValue&, int&) const (content/smil/nsSMILAnimationFunction.cpp:729)

So right now, I think our contract is that ValueFromString() promises to set its "aPreventCachingOfSandwich" outparam whenever it succeeds (returns NS_OK). Based on the failure being in test_SVGxxxList.xhtml, I glanced through a few SVGAnimatedXXXList.cpp::SMILAnimatedXXXList::ValueFromString impls. So far they all seem to honor this contract *except* for one: SVGAnimatedNumberList::SMILAnimatedNumberList::ValueFromString() which never sets aPreventCachingOfSandwich at all. Compare: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedPointList.cpp#187 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedNumberList.cpp#160 (first is good, second is bad) Checking a few more impls for sanity...

Yup, I think that's the only bad one. This patch fixes it.

(Thankfully, this isn't a security problem. The uninitialized-memory-use here just means that we may unnecessarily re-evaluate an animation in samples where we otherwise wouldn't because we know it can't have changed.)

Comment on attachment 503909 [details] [diff] [review] fix ># HG changeset patch ># Parent ae4ee53297b09f50e877afce63ffec6ac47febb1 ># User Daniel Holbert <dholbert@cs.stanford.edu> > >diff --git a/content/svg/content/src/SVGAnimatedNumberList.cpp b/content/svg/content/src/SVGAnimatedNumberList.cpp >--- a/content/svg/content/src/SVGAnimatedNumberList.cpp >+++ b/content/svg/content/src/SVGAnimatedNumberList.cpp >@@ -166,16 +166,17 @@ SVGAnimatedNumberList:: > { > nsSMILValue val(&SVGNumberListSMILType::sSingleton); > SVGNumberListAndInfo *nlai = static_cast<SVGNumberListAndInfo*>(val.mU.mPtr); > nsresult rv = nlai->SetValueFromString(aStr); > if (NS_SUCCEEDED(rv)) { > nlai->SetInfo(mElement); > aValue.Swap(val); > } >+ aPreventCachingOfSandwich = PR_FALSE; > return rv; > } > > nsSMILValue > SVGAnimatedNumberList::SMILAnimatedNumberList::GetBaseValue() const > { > // To benefit from Return Value Optimization and avoid copy constructor calls > // due to our use of return-by-value, we must return the exact same object

(sorry, disregard comment 4 - accidentally hit 'edit as comment' button while requesting approval. thankfully it's a short patch. :))

Landed: http://hg.mozilla.org/mozilla-central/rev/60cbff9cbab0 Thanks for the heads-up on this, Julian!

(In reply to comment #3) > (Thankfully, this isn't a security problem. The uninitialized-memory-use here > just means that we may unnecessarily re-evaluate an animation in samples where > we otherwise wouldn't because we know it can't have changed.) ...though it seems this bug caused visible issues in a reftest -- issues that became fixed when the patch landed. (I'm not quite sure how, but see bug 623405 comment 14.)