Closed Bug 378583 Opened 17 years ago Closed 17 years ago

Large pattern surfaces crash browser

Categories

(Core :: SVG, defect)

defect
Not set
normal

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
Details | Diff | Splinter Review
Attached patch patch (obsolete) — 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
Attached patch address review comments (obsolete) — Splinter Review
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?
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)
Attachment #262800 - Attachment is obsolete: true
Assignee: longsonr → general
Status: ASSIGNED → NEW
Keywords: crash, testcase
Attached patch work in progress (obsolete) — Splinter Review
addresses some review comments but still does not adjust the pattern CTM scaling to compensate for reducing the size of the offscreen surface
Attached patch address review comments (obsolete) — Splinter Review
Assignee: general → longsonr
Attachment #266049 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266139 - Flags: review?(tor)
Attached patch also deal with too small (obsolete) — Splinter Review
For a too small pattern see bug 382242.
Attachment #266139 - Attachment is obsolete: true
Attachment #266450 - Flags: review?(tor)
Attachment #266139 - Flags: review?(tor)
Blocks: 382242
Attachment #266450 - Attachment is obsolete: true
Attachment #266467 - Flags: review?(tor)
Attachment #266450 - Flags: review?(tor)
Attached patch not one of my better days (obsolete) — Splinter Review
Attachment #266467 - Attachment is obsolete: true
Attachment #266475 - Flags: review?(tor)
Attachment #266467 - Flags: review?(tor)
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+
Attached patch address review comment (obsolete) — Splinter Review
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?
(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.
Attached patch address superreview comments (obsolete) — Splinter Review
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)
?
Attached patch address sr comments (obsolete) — Splinter Review
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.
Attached patch address further sr comments (obsolete) — Splinter Review
Also ensures overflow flag set properly.
Attachment #266913 - Attachment is obsolete: true
Attachment #267045 - Flags: superreview?
Attachment #267045 - Flags: superreview? → superreview?(roc)
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+
(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.


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.

Attachment

General

Created:
Updated:
Size: