307742 - Crash in [@ nsSVGGradientFrame::GetStopOffset]

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: 2005-09-09-05, Windows XP SeaMonkey trunk.

I split this out from bug 305021, since this is a different stack trace.

Summary: Crash in nsSVGGradientFrame::GetStopOffset

Steps to Reproduce:

1. Load http://ffsearchplugins.free.fr/bugzilla/SvgGradientCrash.svg
2. Crash ;-(

nsSVGGradientFrame::GetStopOffset 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp,
line 376]
GDIPlusGetStops 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp,
line 80]
GDIPlusLinearGradient 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp,
line 170]
GDIPlusGradient 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp,
line 336]
nsSVGGDIPlusPathGeometry::Render 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusPathGeometry.cpp,
line 454]
nsSVGPathGeometryFrame::PaintSVG 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,
line 271]
nsSVGOuterSVGFrame::Paint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp,
line 842]
nsContainerFrame::PaintChild 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsContainerFrame.cpp,
line 283]
nsContainerFrame::PaintChildren 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsContainerFrame.cpp,
line 228]
nsHTMLContainerFrame::Paint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsHTMLContainerFrame.cpp,
line 84]
CanvasFrame::Paint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsHTMLFrame.cpp,
line 371]
PresShell::Paint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 5645]
nsView::Paint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 316]
nsViewManager::RenderDisplayListElement 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 1468]
nsViewManager::RenderViews 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 1380]
nsViewManager::Refresh 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 930]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2055]
HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1060]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4172]
nsWindow::WindowProc 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1249]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0xb4c0 (0x77d4b4c0)
USER32.dll + 0xb50c (0x77d4b50c)
ntdll.dll + 0xeae3 (0x7c90eae3)
nsWindow::DispatchStarvedPaints 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3992]
USER32.dll + 0xda57 (0x77d4da57)
nsWindow::DispatchPendingEvents 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4030]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4437]
nsWindow::WindowProc 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1249]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppStartup::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/components/startup/src/nsAppStartup.cpp,
line 208]
main 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1738]
WinMain 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1762]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 1

[Scooter Morris]

This was fixed by the checkin for Bug #305021

*** This bug has been marked as a duplicate of 305021 ***

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #1)
> This was fixed by the checkin for Bug #305021
> 
> *** This bug has been marked as a duplicate of 305021 ***

I beg to differ, since I'm still crashing on trunk (note my user-agent's build
date in comment 0, here).  It's _still_ crashing with post-9-08-2005 builds,
which were the first builds after you landed bug 305021 on the trunk.

Am I missing something here?

Comment 3

[Scooter Morris]

Attachment: Possible patch

Oops -- sorry, I got a little confused.  Here is a possible fix to this bug,
but I don't have a Windows/GDI+ build environment to check it.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: LIVE CRASH TESTCASE

Attaching testcase.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: additional/alternative patch - fixes a few other issues

In looking to see why you aren't null checking in the function where the crash
occured I've ended up finding a few other issues.

1) GetStopElement returns 0 for the stop count if there is only one stop, or
rather it will return 1 less than the actual number of stops when used to
actially obtain a stop (as opposed to a stop count as in GetStopCount). This
doesn't matter most of the time since we only need to look at the number of
stops to check they aren't zero, but it does matter in the case of only having
1 stop.

2) As a result of the stop count error, we will try to inherit stops from any
referenced gradient even if we have 1 stop ourselves, which breaks the spec.
This patch should fix that.

3) A few others I've forgotten now.

4) It seems to me that if there is only one stop we won't behave as the spec
says and use a solid color. That would be a separate bug though, and I'll file
that after I test if no one else gets to it first.

5) http://lxr.mozilla.org/seamonkey/search?string=GetStopColorType seems to
showGetStopColorType isn't used except recursively by itself. Can that be
removed? I haven't in this patch.

This patch fixes the crash by putting null checks where they're needed, makes
the code easier to understand, and fixes all the issues I found except for 4
and 5. I don't have time to look into 5 right now to see if there's other code
that should be removed if and when GetStopColorType is removed. E.g. is
mStopColor.mType still relevant? I haven't looked.

Comment 6

[Scooter Morris]

Attachment: Combined patch -- clean up of nsSVGGradientFrame.cpp + checks for no stops in gdi renderer

Comment 7

[Scooter Morris]

Attachment: Oops, forgot to remove reference to GetStopColorType from nsISVGGradient.idl

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using SeaMonkey 1.1a

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917 Mozilla/1.0

Thanks for fixing!

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 196095 [details] [diff] [review]
Oops, forgot to remove reference to GetStopColorType from nsISVGGradient.idl

requesting approval

Comment 10

[Mike Schroepfer]

Comment on attachment 196095 [details] [diff] [review]
Oops, forgot to remove reference to GetStopColorType from nsISVGGradient.idl

Approved for 1.8b5 per bug meeting

Comment 11

[Asa Dotzler [:asa]]

Has this landed on the branch? If not, time is getting very short. If so, please
add the "fixed1.8" keyword. Thanks.

Comment 12

[Jonathan Watt [:jwatt]]

Sorry, my fault.

Comment 13

[Jay Patel [:jay]]

v.fixed on branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20050928 Firefox/1.4, original url and live crash testcase do not crash.