Closed Bug 745676 Opened 12 years ago Closed 8 years ago

Canvas crash [@ mozilla::gfx::AlphaBoxBlur::AlphaBoxBlur]


(Core :: Graphics, defect)

12 Branch
Not set



Tracking Status
firefox12 - wontfix


(Reporter: jruderman, Assigned: eflores)



(Keywords: crash, regression, testcase, Whiteboard: [gfx-noted])

Crash Data


(3 files, 3 obsolete files)

1. Set
  user_pref("", false);
2. Load the testcase


mozilla::gfx::AlphaBoxBlur::AlphaBoxBlur tries to allocate a negative amount of memory, fails, and then dereferences null.

Breakpad fails a little bit: bp-b000808a-b3b8-4cb4-b0e9-313f82120416
Attached file stack trace+
I can reproduce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120415 Firefox/14.0a1 ID:20120415030725

Regression window(m-c)
Not crash:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126093130
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126151450

Regression window(m-i)
Not crash:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126040148
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120125 Firefox/12.0a1 ID:20120126051451

eba8a5ac183f	Edwin Flores — Bug 710521 - Refactor gfxFont to separate out drawing stroke and drawing to path. r=roc
Blocks: 710521
Crash Signature: [@ libsystem_c.dylib@0x27af8] [@ msvcr100.dll@0x101d0]
Keywords: regression
OS: Mac OS X → All
Hardware: x86_64 → All
Version: Trunk → 12 Branch
Assignee: nobody → eflores
Attached patch Fix (obsolete) — Splinter Review
The glyph extents were being unnecessarily calculated in device space which---if the page was being a bit... generous... with transforms---would result in an extents box too big for an IntRect and the world would end.
Attachment #615962 - Flags: review?(roc)
Comment on attachment 615962 [details] [diff] [review]

Review of attachment 615962 [details] [diff] [review]:

Please make the ShadowInitialize comment say that 'extents' is in user space, not device space.
Attachment #615962 - Flags: review?(roc) → review+
Attached patch Fix (obsolete) — Splinter Review
Fixed comment; carrying over r=roc.
Attachment #615962 - Attachment is obsolete: true
Attachment #615993 - Flags: review+
This isn't a top crasher, and doesn't include any evidence that this will be alarming post-release. If that isn't actually the case, please email asap. For now, no need to track for FF12.
Attachment #615993 - Flags: approval-mozilla-central?
Comment on attachment 615993 [details] [diff] [review]

Review of attachment 615993 [details] [diff] [review]:

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ -596,5 @@
>      /**
>       * Initializes the drawing of a shadow onto the canvas. The returned context
>       * should have the shadow shape drawn onto it, and then ShadowFinalize
>       * should be called. The return value is null if an error occurs.
> -     * @param extents The extents of the shadow object, in device space.

are all the callers already assuming user space and this was just documented incorrectly?
Comment on attachment 615993 [details] [diff] [review]

Review of attachment 615993 [details] [diff] [review]:

Actually, now that I check again, no. This patch isn't correct.
Attachment #615993 - Flags: review-
Attachment #615993 - Flags: review+
Attachment #615993 - Flags: approval-mozilla-central?
Attachment #615993 - Flags: approval-mozilla-central-
Attached patch Fix (obsolete) — Splinter Review
Found another caller incorrectly assuming device space. Carry over r=roc
Attachment #615993 - Attachment is obsolete: true
Attachment #616785 - Flags: review+
Comment on attachment 616785 [details] [diff] [review]

Review of attachment 616785 [details] [diff] [review]:

Roc withdrew r+, so I'm withdrawing this one so we don't accidentally land it :)
Attachment #616785 - Flags: review+ → review-
Turns out this was already a bug in this bit of code when filling text, but was only exposed via strokeText once the stroking code was modified to use the same drawing method.

The only thing getting in the way of us crashing before was cairo realising what it was being asked to do was stupid and bounding the bounding box (heh) that it returned.

Will probably just do that.
Attached patch FixSplinter Review
Just bounding the size of the stupid thing. Should tide us over until Apple reveals the new MacBook's 100000000 dpi display.
Attachment #616785 - Attachment is obsolete: true
Attachment #616882 - Flags: review?(roc)
Comment on attachment 616882 [details] [diff] [review]

Review of attachment 616882 [details] [diff] [review]:

Actually, I think the fix should be in gfxAlphaBoxBlur::Init or AlphaBoxBlur::AlphaBoxBlur.

The actual error occurs in AlphaBoxBlur::AlphaBoxBlur in "mRect = IntRect(rect.x, rect.y, rect.width, rect.height);" right? We're just blindly converting floats/doubles to int there. We also have a problem with skipRect there.

I suggest moving gfxUtils::GfxRectToIntRect (or copying it if necessary) into gfx/2d, and calling it from AlphaBoxBlur::AlphaBoxBlur when we do these conversions. If these fail then just return without setting mData.
Attachment #616882 - Flags: review+ → review-
Depends on: 711953
Looks like this bug stalled in review with comment 13 and hasn't gone anywhere in over three years. Looking at crash-stats this still shows up from time to time with (112 crashes in the last 28 days) but the most recent version to be reported is in the Firefox 35 branch. 

Is this bug still relevant or should it be closed?
I am closing this bug as incomplete since the patch seems to have stalled at comment 13 and since there are no reports beyond Firefox 35. Please reopen if this issue is still relevant.
Closed: 8 years ago
Resolution: --- → INCOMPLETE
Whiteboard: [gfx-noted]
You need to log in before you can comment on or make changes to this bug.