580796 - "ASSERTION: Returning unknown unit type" regression from bug 515116

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Returning unknown unit type: 'Not Reached', file svg/content/src/SVGLength.cpp, line 291

mozilla::GetUnitTypeForString [content/svg/content/src/SVGLength.cpp:292]
mozilla::SVGLength::SetValueFromString [content/svg/content/src/SVGLength.cpp:88]
mozilla::SVGLengthList::SetValueFromString [content/svg/content/src/SVGLengthList.cpp:103]
mozilla::SVGAnimatedLengthList::SetBaseValueString [content/svg/content/src/SVGAnimatedLengthList.cpp:52]
nsSVGElement::ParseAttribute [content/svg/content/src/nsSVGElement.cpp:363]
nsSVGStylableElement::ParseAttribute [content/svg/content/src/nsSVGStylableElement.cpp:131]
nsGenericElement::SetAttr [content/base/src/nsGenericElement.cpp:4591]
nsXMLContentSink::AddAttributes [content/xml/document/src/nsXMLContentSink.cpp:1491]
nsXMLContentSink::HandleStartElement [content/xml/document/src/nsXMLContentSink.cpp:1058]
nsXMLContentSink::HandleStartElement [content/xml/document/src/nsXMLContentSink.cpp:986]
nsExpatDriver::HandleStartElement [parser/htmlparser/src/nsExpatDriver.cpp:410]
Driver_HandleStartElement [parser/htmlparser/src/nsExpatDriver.cpp:98]
doContent [parser/expat/lib/xmlparse.c:2465]
contentProcessor [parser/expat/lib/xmlparse.c:2095]
doProlog [parser/expat/lib/xmlparse.c:4075]
prologProcessor [parser/expat/lib/xmlparse.c:3811]
prologInitProcessor [parser/expat/lib/xmlparse.c:3626]
MOZ_XML_Parse [parser/expat/lib/xmlparse.c:1528]
nsExpatDriver::ParseBuffer [parser/htmlparser/src/nsExpatDriver.cpp:1003]
nsExpatDriver::ConsumeToken [parser/htmlparser/src/nsExpatDriver.cpp:1102]
nsParser::Tokenize [parser/htmlparser/src/nsParser.cpp:3090]
nsParser::ResumeParse [parser/htmlparser/src/nsParser.cpp:2322]
nsParser::OnDataAvailable [parser/htmlparser/src/nsParser.cpp:2954]
nsDocumentOpenInfo::OnDataAvailable [uriloader/base/nsURILoader.cpp:323]
nsBaseChannel::OnDataAvailable [netwerk/base/src/nsBaseChannel.cpp:732]
nsInputStreamPump::OnStateTransfer [netwerk/base/src/nsInputStreamPump.cpp:510]
nsInputStreamPump::OnInputStreamReady [netwerk/base/src/nsInputStreamPump.cpp:400]
nsInputStreamReadyEvent::Run [xpcom/io/nsStreamUtils.cpp:113]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessPendingEvents_P [nsThreadUtils.cpp:200]
nsBaseAppShell::NativeEventCallback [widget/src/xpwidgets/nsBaseAppShell.cpp:127]
nsAppShell::ProcessGeckoEvents [widget/src/cocoa/nsAppShell.mm:395]
CoreFoundation + 0x3f0fb
CoreFoundation + 0x3cbbf
CoreFoundation + 0x3c094
CoreFoundation + 0x3bec1
HIToolbox + 0x34f9c
HIToolbox + 0x34d51
HIToolbox + 0x34bd6
AppKit + 0x48a89
-AppKit + 0x482ca
-AppKit + 0xa55b
nsAppShell::Run [widget/src/cocoa/nsAppShell.mm:747]
nsAppStartup::Run [toolkit/components/startup/src/nsAppStartup.cpp:191]
XRE_main [toolkit/xre/nsAppRunner.cpp:3630]
main [browser/app/nsBrowserApp.cpp:158]

Comment 1

[Daniel Holbert [:dholbert]]

Flagging as regression from bug 515116, since the assertion-failure is in code added in that bug.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch and test

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch and test

Comment 4

[Robert Longson [:longsonr]]

The test would be better as a mochitest I think. The code looks fine so r+ on that.

Comment 5

[Jonathan Watt [:jwatt]]

Why would a mochitest be better? I can use a mochitest if you prefer.

Comment 6

[Jonathan Watt [:jwatt]]

The main reason I made it a reftest is because the test is simpler (no HTML or script required).

Comment 7

[Robert Longson [:longsonr]]

What is fundamental to the test is that the value isn't parsed so it comes back as 0. Or alternatively all the changes are in content so the test should be in content too. A mochitest would tell you this straight away directly and it would be easier to figure out what was wrong if something went wrong too.

I think there are existing mochitests you can adapt or extend.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch replacing reftest with mochitest

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note, regressions since Firefox 3.6 should be marked "regression" and requested for blocking.

Comment 10

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/50985e5fa3e3

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> Note, regressions since Firefox 3.6 should be marked "regression" and requested
> for blocking.

(FWIW, I didn't add the "regression" keyword in comment 1 because this isn't technically a regression in the "something that used to work is now broken" sense of the word.  This bug's assertion was added since Firefox 3.6, and Jesse simply found a way to make the new assertion fail.)

(In comment 1, I probably should have said "Flagging as blocking bug 515116" instead of "Flagging as regression from bug 515116" - sorry for the imprecise language there & any confusion w.r.t. regression-status that it caused.)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's a good point. Arguably a testcase that fires an assertion in FF4 that didn't in FF3.6 is also a regression. Personally I think that any bug we have in FF4 that's not in FF3.6 should be treated as a regression.