Closed
Bug 1253590
Opened 8 years ago
Closed 8 years ago
Stack Exhaustion starting at xul!_moz_cairo_region_create_rectangles
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: cbook, Assigned: jwatt)
References
(Blocks 1 open bug, )
Details
(Keywords: crash)
Attachments
(6 files, 2 obsolete files)
14.18 KB,
text/plain
|
Details | |
1.53 MB,
image/svg+xml
|
Details | |
12.23 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
3.76 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
602 bytes,
image/svg+xml
|
Details |
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
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jwatt)
Assignee | ||
Comment 1•8 years ago
|
||
Oh, right, I'd need Linux for cairo. dholbert, would you be able to take a look at this?
Flags: needinfo?(jwatt) → needinfo?(dholbert)
Comment 2•8 years ago
|
||
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•8 years ago
|
||
(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.
Flags: needinfo?(dholbert)
Comment 4•8 years ago
|
||
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
Flags: needinfo?(jwatt)
Comment 5•8 years ago
|
||
(Here's the testcase, for posterity / in case it drops off of Wikipedia.)
Assignee | ||
Comment 6•8 years ago
|
||
(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!!
Flags: needinfo?(jwatt)
Assignee | ||
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
And use clones so that will chew up memory.
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8729876 -
Flags: review?(longsonr)
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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.
Attachment #8729875 -
Flags: review?(longsonr) → review+
Comment 13•8 years ago
|
||
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?
Attachment #8729876 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
Hmm, I guess I could generate the clipPath elements and chain them together using JS in the test file.
Assignee | ||
Comment 17•8 years ago
|
||
I can't repro the crash. Tomcat, or someone who can, can you check that this crashtest also triggers the crash for you?
Flags: needinfo?(cbook)
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8730047 -
Flags: review?(longsonr)
Comment 19•8 years ago
|
||
"testcase 1, from Wikipedia" crashes for me, "live proposed crashtest" doesn't. Win64 trunk build.
Comment 20•8 years ago
|
||
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•8 years ago
|
||
Attachment #8730046 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
[ni=jwatt to incorporate the updated live crashtest into his crashtest patch]
Flags: needinfo?(cbook) → needinfo?(jwatt)
Comment 23•8 years ago
|
||
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.
Attachment #8730053 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8730047 -
Flags: review?(longsonr) → review+
Comment 24•8 years ago
|
||
r+ on the crashtest assuming you make dholbert's changes which crash my Mac.
Reporter | ||
Comment 25•8 years ago
|
||
(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
Reporter | ||
Comment 26•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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
Assignee | ||
Comment 29•8 years ago
|
||
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.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 30•8 years ago
|
||
And yes, since this crash has been around for a long time I don't think we need to worry about uplifting the fix.
Reporter | ||
Comment 32•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•