Closed Bug 580796 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 2 obsolete files)

Attached image testcase (obsolete) —
###!!! 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]
Flagging as regression from bug 515116, since the assertion-failure is in code added in that bug.
Blocks: 515116
Attached patch patch and test (obsolete) — Splinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #465236 - Flags: review?(longsonr)
Attached patch patch and testSplinter Review
Attachment #459176 - Attachment is obsolete: true
Attachment #465236 - Attachment is obsolete: true
Attachment #465238 - Flags: review?(longsonr)
Attachment #465236 - Flags: review?(longsonr)
The test would be better as a mochitest I think. The code looks fine so r+ on that.
Attachment #465238 - Flags: review?(longsonr) → review+
Why would a mochitest be better? I can use a mochitest if you prefer.
The main reason I made it a reftest is because the test is simpler (no HTML or script required).
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.
Summary: "ASSERTION: Returning unknown unit type" → "ASSERTION: Returning unknown unit type" regression from bug 515116
Attachment #495396 - Flags: approval2.0?
Note, regressions since Firefox 3.6 should be marked "regression" and requested for blocking.
Pushed http://hg.mozilla.org/mozilla-central/rev/50985e5fa3e3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.)
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.
You need to log in before you can comment on or make changes to this bug.