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

Status: VERIFIED FIXED

(Core :: SVG, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: 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..

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

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

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.

Comment 3

[Martijn Wargers (dead)]

Attachment: 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?

Comment 4

[Robert Longson [:longsonr]]

I don't think it's a cairo bug its gecko not detecting the recursion. The marked element is an ancestor of the marker.

Comment 5

[Robert Longson [:longsonr]]

Attachment: WIP asynchronously loops rather than crashing

(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.

Comment 6

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

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.

Comment 7

[Robert Longson [:longsonr]]

Attachment: patch

Comment 8

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

Pushed http://hg.mozilla.org/mozilla-central/rev/9246fefef1cc

Need a crashtest here

Comment 9

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

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.)

Comment 10

[Robert Longson [:longsonr]]

(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.

Comment 11

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

(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.

Comment 12

[Robert Longson [:longsonr]]

(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.

Comment 13

[Robert Longson [:longsonr]]

Attachment: patch with updated tests

reftests adjusted for asynchronicity of update

Comment 14

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

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.

Comment 15

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

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.

Comment 16

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 17

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

Pushed http://hg.mozilla.org/mozilla-central/rev/7ab2054c7680

Hmm, I guess we should add a crashtest for this bug.

Comment 18

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090126 Minefield/3.2a1pre

Comment 19

[Daniel Holbert [:dholbert]]

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.

Comment 20

[Marcia Knous [:marcia]]

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.