377892 - Simple clipPath asserting and crashing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Simple w3c test without a reference loop gives these asserts and crashes:

###!!! ASSERTION: reference loop!: 'mFrame->mInUse == PR_FALSE', file /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGClipPathFrame.h, line 91
###!!! ASSERTION: reference loop!: 'mFrame->mInUse == PR_FALSE', file /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGClipPathFrame.h, line 91

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1076413328 (LWP 12393)]
0x41115b85 in nsSVGClipPathFrame::ClipPaint (this=0x0, aContext=0xbf8a82ec, aParent=0xa4c83d0, aMatrix=0xa4d20e8) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp:112
112       if (mInUse) {
(gdb) where 10
#0  0x41115b85 in nsSVGClipPathFrame::ClipPaint (this=0x0, aContext=0xbf8a82ec, aParent=0xa4c83d0, aMatrix=0xa4d20e8) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp:112
#1  0x4113d9df in nsSVGUtils::PaintChildWithEffects (aContext=0xbf8a82ec, aDirtyRect=0xbf8a82f8, aFrame=0xa4c839c) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGUtils.cpp:952
#2  0x41117020 in nsSVGDisplayContainerFrame::PaintSVG (this=0xa4c8000, aContext=0xbf8a82ec, aDirtyRect=0xbf8a82f8) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGContainerFrame.cpp:184
#3  0x4113da65 in nsSVGUtils::PaintChildWithEffects (aContext=0xbf8a82ec, aDirtyRect=0xbf8a82f8, aFrame=0xa4c8000) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGUtils.cpp:964
#4  0x4112c753 in nsSVGOuterSVGFrame::Paint (this=0xa4c6324, aRenderingContext=@0x42681dbc, aDirtyRect=@0xbf8a83d4, aPt=@0xbf8a8360)
    at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:485
#5  0x4112c7de in nsDisplaySVG::Paint (this=0x426729e0, aBuilder=0xbf8a84d4, aCtx=0x42681dbc, aDirtyRect=@0xbf8a83d4) at /home/tor/moz/trunk/mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:376
#6  0x40ae8bdb in nsDisplayList::Paint (this=0x42672a1c, aBuilder=0xbf8a84d4, aCtx=0x42681dbc, aDirtyRect=@0xbf8a83d4) at /home/tor/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:299
#7  0x40ae8e4d in nsDisplayWrapList::Paint (this=0x42672a10, aBuilder=0xbf8a84d4, aCtx=0x42681dbc, aDirtyRect=@0xbf8a83d4) at /home/tor/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:707
#8  0x40ae9d0e in nsDisplayClip::Paint (this=0x42672a10, aBuilder=0xbf8a84d4, aCtx=0x42681dbc, aDirtyRect=@0xbf8a8454) at /home/tor/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:939
#9  0x40ae8bdb in nsDisplayList::Paint (this=0x42672af4, aBuilder=0xbf8a84d4, aCtx=0x42681dbc, aDirtyRect=@0xbf8a8454) at /home/tor/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:299
(More stack frames follow...)

Comment 1

[Robert Longson [:longsonr]]

Works for me on Windows XP, no asserts, no crash.

112   if (mInUse) {
113     NS_WARNING("Clip loop detected!");
114     return NS_OK;
115   }
116   AutoClipPathReferencer clipRef(this);

Line 112 *should* prevent the assert on line 116 from ever being triggered unless the compiler has somehow created the AutoClipPathReferencer object before the if test.

I'm not sure from the traceback how line 112 leads to the assert which is effectively coming from line 116. Are you compiling with optimisation and does it make any difference if you don't?

Comment 2

[Robert Longson [:longsonr]]

Perhaps it is more significant that ClipPaint has a this pointer of 0x00?

Comment 3

[tor]

It appears as though mInUse is now uninitialized before AutoClipPathReferencer examines it for the first time.

Comment 4

[tor]

This a debug build, no optimization.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Does this fix it for you?

Comment 6

[tor]

(In reply to comment #5)
> Created an attachment (id=261971) [details]
> Does this fix it for you?

No - still asserts and crashes.

Comment 7

[tor]

Backing out bug 377015 fixes this bug.

Comment 8

[Robert Longson [:longsonr]]

Attachment: 2nd try

Comment 9

[tor]

You might want to set AutoClipPathReferencer's mFrame to something.

Comment 10

[tor]

Attachment: working fix

Comment 11

[Robert Longson [:longsonr]]

Attachment: 3rd try

Comment 12

[Robert Longson [:longsonr]]

I guess we're finally on the same wavelength.(In reply to comment #9)
> You might want to set AutoClipPathReferencer's mFrame to something.
> 

Doh. Mea Culpa.

Comment 13

[Robert Sayre]

can you check in a reftest for this?

Comment 14

[Robert Longson [:longsonr]]

patch checked in. Will look into a reftest. I assume anything that uses a clipPath is OK.

Comment 15

[Robert Sayre]

(In reply to comment #14)
> patch checked in. Will look into a reftest. I assume anything that uses a
> clipPath is OK.
> 

That's right.

http://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests

Comment 16

[Robert Longson [:longsonr]]

Attachment: reftest

don't think I need a review for this do I? As it's my first attempt though...

Comment 17

[Jonathan Watt [:jwatt]]

Why not just:

  <rect width="100%" height="100%" fill="lime"/>
  <rect width="100%" height="100%" fill="red" clip-path="url(#clipPath)"/>

Comment 18

[Robert Longson [:longsonr]]

mean't to leave it as fixed.

Comment 19

[Robert Longson [:longsonr]]

reftest checked in. No svg reftests are automatically run though as far as I can tell.

Comment 20

[Jonathan Watt [:jwatt]]

Robert, did you miss my question in comment 17?

Comment 21

[Robert Longson [:longsonr]]

I changed the reftest as per your suggestion. Sorry for not making that clear.

Comment 22

[Jonathan Watt [:jwatt]]

Oh, and I was waiting for an answer to my question (to check I hadn't missed something obvious) before going on to suggest a couple of other things. ;-) My other suggestions would be:

* When a test comes from a certain file, please add a comment to the top of the source like the other reftests. I.e.

  <!-- From https://bugzilla.mozilla.org/show_bug.cgi?id=377892 -->

  just after the <title> part. That way, wherever these tests go (W3C?) people can look up the history of them if they get failures or think the test may be invalid (sometimes things tested are subtle).

* Please use the -XX.svg suffix on file names, since as we go on we'll no doubt have many tests where it will make sense to give them the same basic file name as existing tests. I.e. in this case something like clipPath-basic-01.svg would be a good name I think.

Comment 23

[Jonathan Watt [:jwatt]]

s/from a certain file/from a certain bug/

Comment 24

[Robert Longson [:longsonr]]

Attachment: update per comments

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 262632 [details] [diff] [review]
update per comments

yeah, looks good, although like the foreignObject file I did prefer clipPath... rather than clip-path... as the name. where ambiguity exists over whether the naming should be by tag or attribute, we could be consistent and go with tag. doesn't really matter though I guess.

Comment 26

[Robert Longson [:longsonr]]

I'll change it to clipPath on check in.

Comment 27

[Jonathan Watt [:jwatt]]

Cool. In that case reftest will be:

http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/clipPath-basic-01.svg

Comment 28

[Robert Longson [:longsonr]]

reftest changes checked in.