Last Comment Bug 685767 - Factor blur internals out of gfxBlur into a standalone class in gfx/2d
: Factor blur internals out of gfxBlur into a standalone class in gfx/2d
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Joe Drew (not getting mail)
:
Mentors:
: 702683 (view as bug list)
Depends on: 702683 703125 721663 989885
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-08 18:59 PDT by Joe Drew (not getting mail)
Modified: 2014-04-06 12:44 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move innards of gfxAlphaBoxBlur to mozilla::gfx::AlphaBoxBlur (40.98 KB, patch)
2011-09-08 18:59 PDT, Joe Drew (not getting mail)
matt.woodrow: review+
Details | Diff | Splinter Review
work around compiler bug (1.96 KB, patch)
2011-11-17 20:21 PST, Joe Drew (not getting mail)
matt.woodrow: review+
Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2011-09-08 18:59:47 PDT
Created attachment 559364 [details] [diff] [review]
move innards of gfxAlphaBoxBlur to mozilla::gfx::AlphaBoxBlur

In order to implement shadows in the Cairo Azure backend, we need to have Gaussian blur support. The easiest way to do that is to to make the innards of gfxAlphaBoxBlur into their own class that uses it.

This patch does that, creating mozilla::gfx::AlphaBoxBlur.
Comment 1 Ed Morley [:emorley] 2011-11-14 19:37:10 PST
https://hg.mozilla.org/mozilla-central/rev/6ae6d3beeaf4
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-17 10:02:10 PST
Backed out for leaking (see Bug 703125).

https://hg.mozilla.org/mozilla-central/rev/b62e6ee5ba9b
Comment 3 Oleg Romashin (:romaxa) 2011-11-17 10:45:18 PST
With next psuh don't forget bug 702683 patch
Comment 4 Jim Jeffery not reading bug-mail 1/2/11 2011-11-17 12:57:53 PST
Stupid bugzilla - was only adding myself and if closed - Reopening
Comment 5 Joe Drew (not getting mail) 2011-11-17 20:21:49 PST
Created attachment 575377 [details] [diff] [review]
work around compiler bug

It seems that MSVC has a compiler bug that's triggered by AlphaBoxBlur being in the namespace mozilla::gfx. Due to link-time optimization, instead of calling an nsAutoPtr destructor that calls |delete ptr|, it calls a destructor that calls |delete[] ptr|, which doesn't call AlphaBoxBlur's destructor at all.

If we move AlphaBoxBlur to the global namespace, the bug doesn't manifest itself. And since we don't want to do that, if we change from nsAutoPtr to a raw ptr, and call delete on it manually in gfxBlur's destructor, this bug also disappears.

This is an interdiff, on top of the previous patch.
Comment 6 Joe Drew (not getting mail) 2011-11-18 01:51:54 PST
I've just pushed the combined patches on this bug, as well as the fix to bug 702683, to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5549f4202af8
Comment 7 Joe Drew (not getting mail) 2011-11-18 01:53:54 PST
*** Bug 702683 has been marked as a duplicate of this bug. ***
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-18 03:31:53 PST
Joe, can you file a separate bug on investigating if there are any other uses of nsAutoPtr being miscompiled by MSVC?
Comment 9 Ed Morley [:emorley] 2011-11-19 05:16:27 PST
(In reply to Joe Drew (:JOEDREW!) from comment #6)
> I've just pushed the combined patches on this bug, as well as the fix to bug
> 702683, to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5549f4202af8

https://hg.mozilla.org/mozilla-central/rev/5549f4202af8

(But don't forget comment 8 :-))
Comment 10 Joe Drew (not getting mail) 2011-11-21 14:30:03 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Joe, can you file a separate bug on investigating if there are any other
> uses of nsAutoPtr being miscompiled by MSVC?

This is (mentored) bug 704304.
Comment 11 Tetsuro Kato (tete) 2012-03-28 00:53:48 PDT
I think fixed-point division introduced by Bug 633627 has been returned again to integer division by the patch of this bug. Isn't this a regression?
Comment 12 Jeff Muizelaar [:jrmuizel] 2012-03-28 05:31:19 PDT
(In reply to Tetsuro Kato (Tete) from comment #11)
> I think fixed-point division introduced by Bug 633627 has been returned
> again to integer division by the patch of this bug. Isn't this a regression?

Yes it is, can you file a new bug for it?
Comment 13 Tetsuro Kato (tete) 2012-03-28 06:46:55 PDT
Fired a new bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=739969

Note You need to log in before you can comment on or make changes to this bug.