Closed Bug 448903 Opened 16 years ago Closed 16 years ago

Crash [@ nsContextBoxBlur::BoxBlurVertical] with text-shadow

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: ventnor.bugzilla)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file testcase
Loading the testcase makes nsContextBoxBlur::BoxBlurVertical dereference a random address, which usually causes a crash.
Whiteboard: [sg:critical?]
No crash here. OS/Build?

Is your build up-to-date? I remember fixing a crash in BoxBlurVertical before that Martijn found.
Attached patch Patch (obsolete) — Splinter Review
Never mind; its random but I could eventually reproduce this. I get ridiculous values for y for the x[y] operator, but what seemed suspicious was that I was getting frames with height 1. This might explain why you're crashing in vertical blur which is after a perfectly fine horizontal blur.

You can't average 1 value and get anything different, so the easiest thing to do is return early if you only get one pixel in the given direction.

Lets hope I'm right. Can you try again with this patch?
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #332101 - Flags: superreview?(roc)
Attachment #332101 - Flags: review?(roc)
I think the problem is this line in BoxBlurVertical:

pos = PR_MIN(pos, stride - 1);

That constraint makes sense inBoxBlurHorizontal, but in BoxBlurVertical I think pos needs to be restricted by the number of rows instead.
(In reply to comment #3)
> I think the problem is this line in BoxBlurVertical:
> 
> pos = PR_MIN(pos, stride - 1);
> 
> That constraint makes sense inBoxBlurHorizontal, but in BoxBlurVertical I think
> pos needs to be restricted by the number of rows instead.
> 

No, rows is not used in that block but stride is.
The reason I'm suspicious of "pos = PR_MIN(pos, stride - 1);" is that pos is about to be *multiplied* by stride.
Now that I look at it, you're probably right. I wonder why it worked before then...
Attached patch Patch 2Splinter Review
Yeah, that is the cause because I re-read the old blurring code in SVG that I copied this from originally.
Attachment #332101 - Attachment is obsolete: true
Attachment #332108 - Flags: superreview?(roc)
Attachment #332108 - Flags: review?(roc)
Attachment #332101 - Flags: superreview?(roc)
Attachment #332101 - Flags: review?(roc)
Patch 2 fixes the bug for me :)
Attachment #332108 - Flags: superreview?(roc)
Attachment #332108 - Flags: superreview+
Attachment #332108 - Flags: review?(roc)
Attachment #332108 - Flags: review+
roc or jesse, can you push this for me?
Keywords: checkin-needed
Pushed 6524c183be7a. I added Jesse's test as a crashtest --- Michael, it would be slightly more convenient if you added that in your patch next time.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Is it possible to test this without gigantic widths and heights?
Crash Signature: [@ nsContextBoxBlur::BoxBlurVertical]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: