Fix memory leak in GraphicsContextState.

Fix memory leak in GraphicsContextState.

In GraphicsContextState::copy we use placement-new to overwrite the current
object. We need to first call the object's destructor though to not leak any
memory.

BUG=383548
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176106

[Dominik Grewe, 2014-06-12 11:18:49]

ptal

[f(malita), 2014-06-12 13:30:35]

LGTM

Is there a good way to refactor this to avoid placement new? I can't immediately
tell.

[Dominik Grewe, 2014-06-12 13:40:42]

On 2014/06/12 13:30:35, Florin Malita wrote:
> LGTM
> 
> Is there a good way to refactor this to avoid placement new? I can't
immediately
> tell.

The main reason for using placement new was to be able to re-use the copy
constructor code. Previously the copy method would copy each field from one
object to the other, similar to the copy constructor.

[jbroman, 2014-06-12 13:43:07]

https://codereview.chromium.org/335613002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/335613002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.cpp:62:
this->~GraphicsContextState();
Wouldn't it be more straightforward to just make this a member-wise copy (like
an ordinary operator= implementation) rather than destructing and reconstructing
it? I wouldn't expect that to be any slower.

From a simplicity perspective, it would be even nicer to destruct objects as we
stop using them (and so avoid leaking their references to shaders and whatnot
until a sufficiently deep state stack occurs again). That's the default
behaviour of a Vector, so I assume that we decided this was a net performance
win.

[Dominik Grewe, 2014-06-12 13:50:19]

https://codereview.chromium.org/335613002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/335613002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.cpp:62:
this->~GraphicsContextState();
On 2014/06/12 13:43:07, jbroman wrote:
> Wouldn't it be more straightforward to just make this a member-wise copy (like
> an ordinary operator= implementation) rather than destructing and
reconstructing
> it? I wouldn't expect that to be any slower.

We mainly did this to avoid code duplication. But I'm happy to change it back to
simple assignments.


> From a simplicity perspective, it would be even nicer to destruct objects as
we
> stop using them (and so avoid leaking their references to shaders and whatnot
> until a sufficiently deep state stack occurs again). That's the default
> behaviour of a Vector, so I assume that we decided this was a net performance
> win.

I agree, it would be nicer. Not sure why we're doing it but, as you said, maybe
we decided it was faster this way.

[Dominik Grewe, 2014-06-13 08:39:41]

I'm going to land this for now to get rid of the leak.

If any of the owners want me to change anything we discussed above, please let
me know.

[Dominik Grewe, 2014-06-13 08:39:44]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-06-13 08:41:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominikg@chromium.org/335613002/1

[commit-bot: I haz the power, 2014-06-13 09:46:20]

Change committed as 176106