Closed
Bug 378583
Opened 17 years ago
Closed 17 years ago
Large pattern surfaces crash browser
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: longsonr, Assigned: longsonr)
References
Details
(Keywords: crash, testcase)
Attachments
(2 files, 11 obsolete files)
491 bytes,
image/svg+xml
|
Details | |
16.20 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #262629 -
Flags: superreview?(tor)
Attachment #262629 -
Flags: review?(tor)
Comment on attachment 262629 [details] [diff] [review] patch I have a patch switching this over to CreateOffscreenSurface() which returns a gfxASurface that can't be checked for success. Maybe you want to limit the size to some maximum, and scale the pattern down if it exceeds that, like we do with filters? http://lxr.mozilla.org/seamonkey/source/layout/svg/base/src/nsSVGFilterFrame.cpp#207
Assignee | ||
Comment 2•17 years ago
|
||
Assignee: general → longsonr
Attachment #262629 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #262800 -
Flags: superreview?(tor)
Attachment #262800 -
Flags: review?(tor)
Attachment #262629 -
Flags: superreview?(tor)
Attachment #262629 -
Flags: review?(tor)
Maybe you want to make a NS_SVG_OFFSCREEN_MAX_DIMENSION (or more creatively named) macro in nsSVGUtils.h and use it for both filters and patterns? Shouldn't you adjust the pattern CTM scaling to compensate for reducing the size of the offscreen surface?
Assignee | ||
Comment 4•17 years ago
|
||
Comment on attachment 262800 [details] [diff] [review] address review comments Also needs rewriting for thebes changes now. In fact perhaps both patterns and filters chould use nsThebesImage which has size checking built in via nsThebesImage::AllowedImageSize
Attachment #262800 -
Flags: superreview?(tor)
Attachment #262800 -
Flags: review?(tor)
Assignee | ||
Updated•17 years ago
|
Attachment #262800 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Assignee: longsonr → general
Status: ASSIGNED → NEW
Assignee | ||
Comment 5•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 7•17 years ago
|
||
addresses some review comments but still does not adjust the pattern CTM scaling to compensate for reducing the size of the offscreen surface
Assignee | ||
Comment 8•17 years ago
|
||
Assignee: general → longsonr
Attachment #266049 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266139 -
Flags: review?(tor)
Assignee | ||
Comment 9•17 years ago
|
||
For a too small pattern see bug 382242.
Attachment #266139 -
Attachment is obsolete: true
Attachment #266450 -
Flags: review?(tor)
Attachment #266139 -
Flags: review?(tor)
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #266450 -
Attachment is obsolete: true
Attachment #266467 -
Flags: review?(tor)
Attachment #266450 -
Flags: review?(tor)
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #266467 -
Attachment is obsolete: true
Attachment #266475 -
Flags: review?(tor)
Attachment #266467 -
Flags: review?(tor)
Comment 12•17 years ago
|
||
Comment on attachment 266475 [details] [diff] [review] not one of my better days >@@ -280,23 +280,53 @@ nsSVGPatternFrame::PaintPattern(gfxASurf > >+ // 0 disables rendering, < 0 is an error >+ if (surfaceWidth < 0.5f || surfaceHeight < 0.5f) >+ return NS_ERROR_FAILURE; >+ >+ gfxIntSize surfaceSize = >+ gfxIntSize(PRInt32(PR_MIN(PR_INT32_MAX, surfaceWidth) + 0.5f), >+ PRInt32(PR_MIN(PR_INT32_MAX, surfaceHeight) + 0.5f)); I think you want the +0.5 inside the PR_MINs.
Attachment #266475 -
Flags: review?(tor) → review+
Assignee | ||
Comment 13•17 years ago
|
||
Attachment #266475 -
Attachment is obsolete: true
Attachment #266593 -
Flags: superreview?(roc)
+ PRInt32 filterResX = PR_MIN(PR_INT32_MAX, PRInt32(s1 * width + 0.5)); + PRInt32 filterResY = PR_MIN(PR_INT32_MAX, PRInt32(s2 * height + 0.5)); I'm confused. PR_MIN(PR_INT32_MAX, PRInt32(x)) will always return x, right?
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #14) > + PRInt32 filterResX = PR_MIN(PR_INT32_MAX, PRInt32(s1 * width + 0.5)); > + PRInt32 filterResY = PR_MIN(PR_INT32_MAX, PRInt32(s2 * height + 0.5)); > > I'm confused. PR_MIN(PR_INT32_MAX, PRInt32(x)) will always return x, right? > Erm, yes. Oops.
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #266593 -
Attachment is obsolete: true
Attachment #266752 -
Flags: superreview?(roc)
Attachment #266593 -
Flags: superreview?(roc)
Can't you use a helper function here to share some code? something like static gfxIntSize ConvertToSurfaceSize(const gfxSize& aSize, PRBool* aResultIsExact) ?
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #266752 -
Attachment is obsolete: true
Attachment #266752 -
Flags: superreview?(roc)
+ nsSVGUtils::ConvertToSurfaceSize(gfxSize(s1 * width + 0.5, s2 * height + 0.5), Do you still need these 0.5s here? I'm not sure why. /* + * Ensure surfaces do not overflow or otherwise use too much memory. + */ Better comment please.
Assignee | ||
Comment 20•17 years ago
|
||
Also ensures overflow flag set properly.
Attachment #266913 -
Attachment is obsolete: true
Attachment #267045 -
Flags: superreview?
Assignee | ||
Updated•17 years ago
|
Attachment #267045 -
Flags: superreview? → superreview?(roc)
Assignee | ||
Comment 21•17 years ago
|
||
Attachment #267045 -
Attachment is obsolete: true
Attachment #267490 -
Flags: superreview?(roc)
Attachment #267045 -
Flags: superreview?(roc)
Comment on attachment 267490 [details] [diff] [review] correct mixed up height and width in filterRes for alpha + nsRect(0, 0, filterRes.width, filterRes.height), For future reference, I think nsRect(nsPoint(0, 0), filterRes) is slightly more elegant.
Attachment #267490 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 23•17 years ago
|
||
(In reply to comment #22) > (From update of attachment 267490 [details] [diff] [review]) > + nsRect(0, 0, filterRes.width, filterRes.height), > > For future reference, I think nsRect(nsPoint(0, 0), filterRes) is slightly more > elegant. > Unfortunately though that doesn't compile. One of the nsRect constructors takes an nsPoint and an nsSize. filterRes is a gfxIntSize, however.
Assignee | ||
Comment 24•17 years ago
|
||
attachment (id=267490) [details] checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•