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




6 years ago
a year ago


(Reporter: Jesse Ruderman, Assigned: eflores)


(Blocks: 1 bug, {crash, regression, testcase})

12 Branch
crash, regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox12- wontfix)


(Whiteboard: [gfx-noted], crash signature)


(3 attachments, 3 obsolete attachments)



6 years ago
Created attachment 615247 [details]
testcase (crashes Firefox when loaded)

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

Comment 1

6 years ago
Created attachment 615248 [details]
stack trace+

Comment 2

6 years ago
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


6 years ago
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
tracking-firefox12: --- → ?
Assignee: nobody → eflores
Created attachment 615962 [details] [diff] [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+
Created attachment 615993 [details] [diff] [review]

Fixed comment; carrying over r=roc.
Attachment #615962 - Attachment is obsolete: true
Attachment #615993 - Flags: review+
Keywords: checkin-needed

Comment 6

6 years ago
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.
status-firefox12: --- → wontfix
tracking-firefox12: ? → -
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-
Created attachment 616785 [details] [diff] [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.
Created attachment 616882 [details] [diff] [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-


6 years ago
Depends on: 711953
Blocks: 754663


6 years ago
Duplicate of this bug: 778434
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.
Last Resolved: a year ago
Resolution: --- → INCOMPLETE
Whiteboard: [gfx-noted]
You need to log in before you can comment on or make changes to this bug.