Closed Bug 472135 Opened 11 years ago Closed 11 years ago

Crash [@ nsSVGLineElement::GetMarkPoints] with marker stuff and line

Categories

(Core :: SVG, defect, P2, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(3 files, 4 obsolete files)

Attached image testcase
See testcase, which crashes current trunk build on load.

This regressed between 2008-10-10 and 2008-10-11:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-10+04%3A00%3A00&enddate=2008-10-11+05%3A00%3A00
I think a regression from bug 309220.

http://crash-stats.mozilla.com/report/index/e82a59d9-a628-4b55-b19d-6e1b02090105?p=1
0  	mozcrt19.dll  	_ctrandisp2  	
1 	xul.dll 	nsSVGLineElement::GetMarkPoints 	content/svg/content/src/nsSVGLineElement.cpp:180
2 	xul.dll 	nsSVGPathGeometryFrame::GetCoveredRegion 	layout/svg/base/src/nsSVGPathGeometryFrame.cpp:213
3 	xul.dll 	nsSVGPathGeometryFrame::UpdateCoveredRegion 	layout/svg/base/src/nsSVGPathGeometryFrame.cpp:275
4 	xul.dll 	nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion 	layout/svg/base/src/nsSVGOuterSVGFrame.cpp:658
5 	xul.dll 	nsSVGMarkerProperty::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:229
6 	xul.dll 	nsSVGRenderingObserver::InvalidateViaReferencedFrame 	layout/svg/base/src/nsSVGEffects.cpp:140
7 	xul.dll 	nsSVGRenderingObserverList::InvalidateAll 	layout/svg/base/src/nsSVGEffects.cpp:407
8 	xul.dll 	nsSVGEffects::InvalidateRenderingObservers 	layout/svg/base/src/nsSVGEffects.cpp:468
9 	xul.dll 	nsSVGRenderingObserver::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:129
10 	xul.dll 	nsSVGMarkerProperty::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:220
11 	xul.dll 	nsSVGRenderingObserver::InvalidateViaReferencedFrame 	layout/svg/base/src/nsSVGEffects.cpp:140
12 	xul.dll 	nsSVGRenderingObserverList::InvalidateAll 	layout/svg/base/src/nsSVGEffects.cpp:407
13 	xul.dll 	nsSVGEffects::InvalidateRenderingObservers 	layout/svg/base/src/nsSVGEffects.cpp:468
14 	xul.dll 	nsSVGRenderingObserver::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:129
15 	xul.dll 	nsSVGMarkerProperty::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:220
16 	xul.dll 	nsSVGRenderingObserver::InvalidateViaReferencedFrame 	layout/svg/base/src/nsSVGEffects.cpp:140
17 	xul.dll 	nsSVGRenderingObserverList::InvalidateAll 	layout/svg/base/src/nsSVGEffects.cpp:407
18 	xul.dll 	nsSVGEffects::InvalidateRenderingObservers 	layout/svg/base/src/nsSVGEffects.cpp:468
19 	xul.dll 	nsSVGRenderingObserver::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:129
20 	xul.dll 	nsSVGMarkerProperty::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:220
etc..
Flags: blocking1.9.1?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #355439 - Flags: superreview?(roc)
Attachment #355439 - Flags: review?(roc)
OS: Windows XP → All
Hardware: x86 → All
I don't think this is the correct fix. I think it's vulnerable to loops created with extra levels of indirection.

What's *supposed* to prevent this loop is that nsSVGRenderingObserver::InvalidateViaReferencedFrame is supposed to clear mReferencedFrame, which should be preventing another call to InvalidateViaReferencedFrame on this observer from doing anything. I'm not sure why that's not working here.
Flags: blocking1.9.1? → blocking1.9.1+
Attached image testcase2
This is a similar testcase, but crashes in a different way:
http://crash-stats.mozilla.com/report/index/736bf030-e298-4dc4-998c-779192090111?p=1
0  	xul.dll  	_cairo_spline_error_squared  	 gfx/cairo/cairo/src/cairo-spline.c:198
1 	xul.dll 	_cairo_spline_decompose_into 	gfx/cairo/cairo/src/cairo-spline.c:252
2 	xul.dll 	_cairo_spline_decompose_into 	gfx/cairo/cairo/src/cairo-spline.c:257
3 	xul.dll 	_cairo_spline_decompose_into 	gfx/cairo/cairo/src/cairo-spline.c:257
4 	xul.dll 	_cairo_spline_decompose_into 	gfx/cairo/cairo/src/cairo-spline.c:257
5 	xul.dll 	_cairo_spline_decompose 	gfx/cairo/cairo/src/cairo-spline.c:278
6 	xul.dll 	_cpf_curve_to 	gfx/cairo/cairo/src/cairo-path-fixed.c:739
7 	xul.dll 	_cairo_path_fixed_interpret 	gfx/cairo/cairo/src/cairo-path-fixed.c:524
8 	xul.dll 	_cairo_path_fixed_interpret_flat 	gfx/cairo/cairo/src/cairo-path-fixed.c:788
9 	xul.dll 	_cairo_path_fixed_bounds 	gfx/cairo/cairo/src/cairo-path-bounds.c:157
10 	xul.dll 	_cairo_gstate_path_extents 	gfx/cairo/cairo/src/cairo-gstate.c:789
11 	xul.dll 	_moz_cairo_path_extents 	gfx/cairo/cairo/src/cairo.c:1917
12 	xul.dll 	gfxContext::GetUserPathExtent 	gfx/thebes/src/gfxContext.cpp:767
13 	xul.dll 	nsSVGPathGeometryFrame::UpdateCoveredRegion 	layout/svg/base/src/nsSVGPathGeometryFrame.cpp:260
14 	xul.dll 	nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion 	layout/svg/base/src/nsSVGOuterSVGFrame.cpp:658
15 	xul.dll 	nsSVGMarkerProperty::DoUpdate 	layout/svg/base/src/nsSVGEffects.cpp:229
etc..

This looks a bit like the crashes in bug 435756. Is this perhaps a bug in cairo?
I don't think it's a cairo bug its gecko not detecting the recursion. The marked element is an ancestor of the marker.
(In reply to comment #2)
> 
> What's *supposed* to prevent this loop is that
> nsSVGRenderingObserver::InvalidateViaReferencedFrame is supposed to clear
> mReferencedFrame, which should be preventing another call to
> InvalidateViaReferencedFrame on this observer from doing anything. I'm not sure
> why that's not working here.

nsSVGRenderingObserver::InvalidateViaReferencedFrame calls DoUpdate which ends up in nsSVGMarkerProperty::DoUpdate. This calls UpdateAndInvalidateCoveredRegion which calls UpdateCoveredRegion and this puts back the referenced frame pointer in order to determine the covered region as that is affected by the presence of markers.

The new patch doesn't crash but it does suffer from an asynchronous repaint loop so the CPU runs at 100%. I don't know how to fix that, suggestions appreciated.
I guess we're calling nsSVGMarkerProperty::DoUpdate during painting somehow? You'd need to find the call stack for that and figure out how to avoid calling it if things are already up to date.
Attached patch patch (obsolete) — Splinter Review
Attachment #355439 - Attachment is obsolete: true
Attachment #356634 - Attachment is obsolete: true
Attachment #357073 - Flags: superreview?(roc)
Attachment #357073 - Flags: review?(roc)
Attachment #355439 - Flags: superreview?(roc)
Attachment #355439 - Flags: review?(roc)
Attachment #357073 - Flags: superreview?(roc)
Attachment #357073 - Flags: superreview+
Attachment #357073 - Flags: review?(roc)
Attachment #357073 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/9246fefef1cc

Need a crashtest here
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Backed out. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232184917.1232189138.19404.gz#err1 had two failures. One is an unexpected-pass on dynamic-filter-contents-01.svg, which you predicted, and is fine, although I don't quite understand why this patch fixes that. But dynamic-marker-01.svg failed, which seems like a real problem. (It didn't fail on the Windows test though, so it might be random.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 191 landing]
(In reply to comment #9)
> Backed out.
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232184917.1232189138.19404.gz#err1
> had two failures. One is an unexpected-pass on dynamic-filter-contents-01.svg,
> which you predicted, and is fine, although I don't quite understand why this
> patch fixes that.

filters already use the async method to refresh which needs the change in nsCSSFrameConstructor to work properly.

 But dynamic-marker-01.svg failed, which seems like a real
> problem. (It didn't fail on the Windows test though, so it might be random.)

Not sure I can do anything about that till my Mac comes. It works for me but then I only have Windows at the moment.
(In reply to comment #10)
> filters already use the async method to refresh which needs the change in
> nsCSSFrameConstructor to work properly.

Ah, of course.

> > But dynamic-marker-01.svg failed, which seems like a real
> > problem. (It didn't fail on the Windows test though, so it might be random.)
> 
> Not sure I can do anything about that till my Mac comes. It works for me but
> then I only have Windows at the moment.

If I get a chance I'll try to reproduce it in my Windows VM. It might just be random though, try varying the timeouts. You could also try converting the test to use the MozReftestInvalidate event instead of setTimeout.
(In reply to comment #9)
> patch fixes that. But dynamic-marker-01.svg failed, which seems like a real
> problem. (It didn't fail on the Windows test though, so it might be random.)

You confused me there. The Windows tinderbox was the only one to fail, Mac and Linux passed. This didn't fail for me on Windows although I only ran the reftests once with the patch in. I'll try running the test locally some more times to see if I can get it to fail.
Attached patch patch with updated tests (obsolete) — Splinter Review
reftests adjusted for asynchronicity of update
Attachment #357073 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs landing]
As I mentioned in the other bug, I'd really like the "not Gecko" timeout to be much larger, say 5000ms, both to ensure the timeout does not fire before MozReftestInvalidate (on a slow machine, like a slow test VM) and to ensure that the test is reliable on non-Gecko.
Hmm, and actually, the way these tests are doing it is wrong. You should be running doTest in response to MozReftestInvalidate and removing reftest-wait inside MozReftestInvalidate should work fine.

The patch you had in your other bug might have the same problem.
Attachment #358002 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/7ab2054c7680

Hmm, I guess we should add a crashtest for this bug.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Depends on: 475193
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090126 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Pushed to 1.9.1 as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c126be164faa

Note: These reftest.list changes weren't applicable to the 1.9.1 changeset...
  -fails == dynamic-filter-contents-01.svg dynamic-filter-contents-01-ref.svg # bug 471631
  +== dynamic-filter-contents-01.svg dynamic-filter-contents-01-ref.svg
...because that test was _already_ marked as passing on 1.9.1. (The "fails" annotation had been added to mozilla-central as part of Bug 471365's patch, but that patch doesn't seem to be intended for 1.9.1)

Aside from that, the 1.9.1 changeset is the same as the one that landed on mozilla-central.
Whiteboard: [needs 191 landing]
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090409 Shiretoko/3.5b4pre and  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090409 Shiretoko/3.5b4pre. Adding keyword.
Crash Signature: [@ nsSVGLineElement::GetMarkPoints]
You need to log in before you can comment on or make changes to this bug.