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

RESOLVED INCOMPLETE

Status

()

Core
Graphics
--
critical
RESOLVED INCOMPLETE
6 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: eflores)

Tracking

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

12 Branch
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12- wontfix)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

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

1. Set
  user_pref("gfx.canvas.azure.enabled", false);
2. Load the testcase

Result:

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
(Reporter)

Comment 1

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

Comment 2

6 years ago
I can reproduce
bp-84df32f3-a418-4d5f-a4b8-6d0072120416
http://hg.mozilla.org/mozilla-central/rev/0d871550085e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120415 Firefox/14.0a1 ID:20120415030725


Regression window(m-c)
Not crash:
http://hg.mozilla.org/mozilla-central/rev/7cdb5f5d38c6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126093130
Crashes:
http://hg.mozilla.org/mozilla-central/rev/a82c9700c673
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126151450
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7cdb5f5d38c6&tochange=a82c9700c673


Regression window(m-i)
Not crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba57f7b6a2f3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 ID:20120126040148
Crashes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9b70467dfec0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120125 Firefox/12.0a1 ID:20120126051451
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ba57f7b6a2f3&tochange=9b70467dfec0

Suspected:
eba8a5ac183f	Edwin Flores — Bug 710521 - Refactor gfxFont to separate out drawing stroke and drawing to path. r=roc

Updated

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]
Fix

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]
Fix

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]
Fix

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 release-mgmt@mozilla.com 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]
Fix

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]
Fix

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]
Fix

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]
Fix

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]
Fix

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]
Fix

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-

Updated

6 years ago
Depends on: 711953
Blocks: 754663

Updated

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.
Status: NEW → RESOLVED
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.