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

Status: RESOLVED INCOMPLETE

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack trace+

Comment 2

[Alice0775 White]

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

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix

Fixed comment; carrying over r=roc.

Comment 6

[Alex Keybl [:akeybl]]

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 [e-mail] asap. For now, no need to track for FF12.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

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 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix

Found another caller incorrectly assuming device space. Carry over r=roc

Comment 10

[Bas Schouten (:bas.schouten)]

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 :)

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix

Just bounding the size of the stupid thing. Should tide us over until Apple reveals the new MacBook's 100000000 dpi display.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[u279076]

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?

Comment 16

[u279076]

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.