1253590 - Stack Exhaustion starting at xul!_moz_cairo_region_create_rectangles

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: windows debug build stack with todays m-c tip

Found by bughunter and reproduced on m-c tip and win 7 debug build on https://upload.wikimedia.org/wikipedia/commons/0/02/Libya_-_Location_Map_%282013%29_-_LBY_-_UNOCHA.svg

Steps to reproduce:
-> Load https://upload.wikimedia.org/wikipedia/commons/0/02/Libya_-_Location_Map_%282013%29_-_LBY_-_UNOCHA.svg
--> Crash

Comment 1

[Jonathan Watt [:jwatt]]

Oh, right, I'd need Linux for cairo. dholbert, would you be able to take a look at this?

Comment 2

[Daniel Holbert [:dholbert]]

No crash for me, though I do hang for a few seconds, and my non-e10s session takes ~2 seconds to switch tabs to this SVG file.

Comment 3

[Daniel Holbert [:dholbert]]

(Comment 2 was using a Nightly build, but I get the same results [with a bit longer hang-times times] in a Linux debug build. Maybe the stack is smaller on Win7 as compared to Linux? Anyway, presumably we can debug this even without triggering the crash.)

From attaching in GDB inside of a nested call to GetClipMask, it seems we're taking the "clipPathThatClipsClipPath" branch and making a recursive call to GetClipMask.

Looking at the testcase, this is because there's indeed some extremely-deeply-nested clip-paths:

<clipPath id="SVGID_4991_">
	<use xlink:href="#SVGID_69_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_4992_" clip-path="url(#SVGID_4991_)">
	<use xlink:href="#SVGID_70_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_4993_" clip-path="url(#SVGID_4992_)">
	<use xlink:href="#SVGID_71_"  overflow="visible"/>
</clipPath>
[...]
<clipPath id="SVGID_9908_" clip-path="url(#SVGID_9907_)">
	<use xlink:href="#SVGID_4986_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_9909_" clip-path="url(#SVGID_9908_)">
	<use xlink:href="#SVGID_4987_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_9910_" clip-path="url(#SVGID_9909_)">
	<use xlink:href="#SVGID_4988_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_9911_" clip-path="url(#SVGID_9910_)">
	<use xlink:href="#SVGID_4989_"  overflow="visible"/>
</clipPath>
<clipPath id="SVGID_9912_" clip-path="url(#SVGID_9911_)">
	<use xlink:href="#SVGID_4990_"  overflow="visible"/>
</clipPath>

Notice that each <clipPath> element has clip-path="url(#[ID-of-previous-clip-path])".

So, this seems doomed to cause stack exhaustion on platforms that have small stacks, unless we restructure this using tail recursion or something like that.

Comment 4

[Daniel Holbert [:dholbert]]

I'll gently punt this back over to jwatt, since he touched this code most recently in bug 1223604, and he's more likely than I am to know if there's anything we can do here. (though I'm not sure there is much we can do here.)

The recursive call that we'd need to eliminate/unfold here (if possible) is:
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGClipPathFrame.cpp?rev=e500c5a05778&mark=140-142#129

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1, from Wikipedia (WARNING: might crash your browser)

(Here's the testcase, for posterity / in case it drops off of Wikipedia.)

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Looking at the testcase, this is because there's indeed some
> extremely-deeply-nested clip-paths:
> 
> <clipPath id="SVGID_4991_">
> 	<use xlink:href="#SVGID_69_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_4992_" clip-path="url(#SVGID_4991_)">
> 	<use xlink:href="#SVGID_70_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_4993_" clip-path="url(#SVGID_4992_)">
> 	<use xlink:href="#SVGID_71_"  overflow="visible"/>
> </clipPath>
> [...]
> <clipPath id="SVGID_9908_" clip-path="url(#SVGID_9907_)">
> 	<use xlink:href="#SVGID_4986_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_9909_" clip-path="url(#SVGID_9908_)">
> 	<use xlink:href="#SVGID_4987_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_9910_" clip-path="url(#SVGID_9909_)">
> 	<use xlink:href="#SVGID_4988_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_9911_" clip-path="url(#SVGID_9910_)">
> 	<use xlink:href="#SVGID_4989_"  overflow="visible"/>
> </clipPath>
> <clipPath id="SVGID_9912_" clip-path="url(#SVGID_9911_)">
> 	<use xlink:href="#SVGID_4990_"  overflow="visible"/>
> </clipPath>
> 
> Notice that each <clipPath> element has
> clip-path="url(#[ID-of-previous-clip-path])".

WTF!!! That clip path chain is like 5000 clip paths long!!! That's so insane that I never even considered the testcase might be doing something like that. I should know by now that Adobe Illustrator will stop at absolutely nothing to produce the most insane SVG output conceivable to man!!

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I'll gently punt this back over to jwatt, since he touched this code most
> recently in bug 1223604, and he's more likely than I am to know if there's
> anything we can do here.

Yeah, I'm probably the person to look at this. Thanks for investigating. :)

Comment 8

[Robert Longson [:longsonr]]

And use clones so that will chew up memory.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Generalize AutoReferenceLoopDetector to allow it to be used to limit reference chain lengths

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Use the new AutoReferenceLimiter helper to limit clip path reference chain lengths

Comment 11

[Jonathan Watt [:jwatt]]

It's probably not surprising at all, but these patches have no visual affect on the rendering of the SVG that causes this crash.

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 8729875 [details] [diff] [review]
part 1 - Generalize AutoReferenceLoopDetector to allow it to be used to limit reference chain lengths


>+  explicit AutoReferenceLimiter(int16_t* aRefCounter,
>+                                int16_t aMaxReferenceCount)

as far as I know the explicit keyword does not make sense for constructors with > 1 non-default arguments.

r=me with that fixed unless you know otherwise.

Comment 13

[Robert Longson [:longsonr]]

Comment on attachment 8729876 [details] [diff] [review]
part 2 - Use the new AutoReferenceLimiter helper to limit clip path reference chain lengths

check in a crashtest too?

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #12)
> as far as I know the explicit keyword does not make sense for constructors
> with > 1 non-default arguments.

Good catch. You're quite right, that's just left over from when I copied over the AutoReferenceLoopDetector code before rewriting it.

Probably I should put this class in its own file too in retrospect.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #13)
> check in a crashtest too?

I'll see what size it turns out. If it's too big I'm a bit reluctant to given the cost benefit ratio of adding a big file to cater for a case that we've not seen a report on for years.

Comment 16

[Jonathan Watt [:jwatt]]

Hmm, I guess I could generate the clipPath elements and chain them together using JS in the test file.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: live proposed crashtest

I can't repro the crash. Tomcat, or someone who can, can you check that this crashtest also triggers the crash for you?

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Crashtest for long clipPath reference chains

Comment 19

[Ryan VanderMeulen [:RyanVM]]

"testcase 1, from Wikipedia" crashes for me, "live proposed crashtest" doesn't. Win64 trunk build.

Comment 20

[Daniel Holbert [:dholbert]]

I get same results as RyanVM in my Win64 Nightly - the live proposed crashtest doesn't seem to test the bug. (I think because it mistakenly uses "xlink:href" on each clipPath element to reference the next, when it should really be using the "clip-path" attribute as in comment 3).

I've made a modified (and somewhat simplified) version of the attached live crashtest, which *does* crash for me. I'll attach that.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: live proposed crashtest, v2 (actually triggers the crash on Win64)

Comment 22

[Daniel Holbert [:dholbert]]

[ni=jwatt to incorporate the updated live crashtest into his crashtest patch]

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: live proposed crashtest, v3 (actually triggers the crash on Win64 & Linux64)

Actually, if I increase the depth by an order of magnitude (to 100,000) then I can get my Linux64 Nightly to crash as well.

Here's an updated version with that tweak.

Comment 24

[Robert Longson [:longsonr]]

r+ on the crashtest assuming you make dholbert's changes which crash my Mac.

Comment 25

[Carsten Book [:Tomcat]]

(In reply to Jonathan Watt [:jwatt] from comment #17)
> Created attachment 8730046 [details]
> live proposed crashtest
> 
> I can't repro the crash. Tomcat, or someone who can, can you check that this
> crashtest also triggers the crash for you?

yeah both testcases (the live one and the from wikimedia) crashed my debug build

Comment 26

[Carsten Book [:Tomcat]]

also note: according to bughunter regarding to crashes also beta and aurora are affected, so we might want to uplift this i think

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Carsten Book [:Tomcat] from comment #26)
> also note: according to bughunter regarding to crashes also beta and aurora
> are affected, so we might want to uplift this i think

Hmm... this isn't a regression (or not a recent one) -- I can reproduce it using a ~4-year-old Nightly that I've got lying around (2012-06-25).

So, unless the crash volume is high, I don't see any strong pressure to shoehorn this into beta & take the risk of causing regressions, unless the crash volume were particularly high. Might be worth an Aurora backport, but I could go either way on that.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec19ec66b5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e3295ba915
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad88856cdfdc

Comment 29

[Jonathan Watt [:jwatt]]

I dropped the ball on this bug due to wanting to figure out if it's really worth running the test given how much time it adds to test runs.  I haven't really got the time to think about that currently so for now I've landed everything with the test disabled to stop it rotting/being forgotten, and we can revisit enabling the test in bug 1259356.

Comment 30

[Jonathan Watt [:jwatt]]

And yes, since this crash has been around for a long time I don't think we need to worry about uplifting the fix.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/406c780a3817

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ec19ec66b5d
https://hg.mozilla.org/mozilla-central/rev/d6e3295ba915
https://hg.mozilla.org/mozilla-central/rev/ad88856cdfdc
https://hg.mozilla.org/mozilla-central/rev/406c780a3817