status_icons: Change StatusTray to use ScopedVector.

status_icons: Change StatusTray to use ScopedVector.

R=atwilson@chromium.org
TBR=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138037

[Andrew T Wilson (Slow), 2012-05-17 23:42:01]

On 2012/05/17 23:23:02, tfarina wrote:

LGTM, but looks like some test failures?

[tfarina, 2012-05-17 23:55:03]

On Thu, May 17, 2012 at 8:42 PM,  <atwilson@chromium.org> wrote:
> On 2012/05/17 23:23:02, tfarina wrote:
>
> LGTM, but looks like some test failures?
>
Yeah, looking into this now. :/

The error is: "pointer being freed was not allocated".

-- 
Thiago

[Andrew T Wilson (Slow), 2012-05-18 00:06:17]

I guess ScopedVector.erase() frees the object so you don't have to have
RemoveStatusIcon call delete any more.

[tfarina, 2012-05-18 00:15:49]

On Thu, May 17, 2012 at 9:06 PM,  <atwilson@chromium.org> wrote:
> I guess ScopedVector.erase() frees the object so you don't have to have
> RemoveStatusIcon call delete any more.
>
That is right.

Because it does:
delete *position;
return v.erase(position);

But we do first:
delete *i;

And then ScopedVector().erase() does:
delete *position; // So we are double freeing it.

I think this is subtle, because if you access the erase function using
dot (.) you get the erase implementation of ScopedVector, but using ->
you will get the default implementation from std::vector.

So the two of possible fixes are:

1- Remove the "delete *i;" and we are fine.
2- Keep the "delete *i;" and switch to status_icons_->erase(i);

A third option that is the same of 2, but deleting the pointer after erase:
status_icons_->erase(i);
delete icon;

I'm waiting for the compilation to finish to try it out.

-- 
Thiago

[tfarina, 2012-05-18 00:52:16]

On 2012/05/18 00:06:17, Andrew T Wilson wrote:
> I guess ScopedVector.erase() frees the object so you don't have to have
> RemoveStatusIcon call delete any more.

Patch set 2 fixes the unittest. I went with just removing the "delete *i;".