Correctly recognize emptiness of IDMap.

Correctly recognize emptiness of IDMap.

This prevents a leak of RenderProcessHost, and possibly other
bad bugs.

TEST=Added to base_unittests; also existing browser and unit tests.
BUG=35571
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39084

[Paweł Hajdan Jr., 2010-02-13 18:22:25]

Please also advise whether this should land in 249 and/or 307 branch.

[brettw, 2010-02-14 18:13:03]

LGTM. Is IsEmpty called during normal running? (I'm at home so can't
easily check). If it is used for something useful, I'd say land it on
the branch. If it's just this test, trunk should be fine.

Brett

On Sat, Feb 13, 2010 at 10:22 AM,  <phajdan.jr@chromium.org> wrote:
> Reviewers: brettw, darin, willchan,
>
> Message:
> Please also advise whether this should land in 249 and/or 307 branch.
>
> Description:
> Correctly recognize emptiness of IDMap.
>
> This prevents a leak of RenderProcessHost, and possibly other
> bad bugs.
>
> TEST=Added to base_unittests; also existing browser and unit tests.
> BUG=35571
>
> Please review this at http://codereview.chromium.org/604048
>
> Affected files:
>  M base/id_map.h
>  M base/id_map_unittest.cc
>
>
> Index: base/id_map.h
> diff --git a/base/id_map.h b/base/id_map.h
> index
>
b108dbd8755f706c49cecd555cac7815e0bf8ba7..acfba42bb94a61b1beeeeb128f2a7abe9090a603
> 100644
> --- a/base/id_map.h
> +++ b/base/id_map.h
> @@ -82,7 +82,7 @@ class IDMap {
>   }
>
>   bool IsEmpty() const {
> -    return data_.empty();
> +    return size() == 0u;
>   }
>
>   T* Lookup(KeyType id) const {
> @@ -93,7 +93,7 @@ class IDMap {
>   }
>
>   size_t size() const {
> -    return data_.size();
> +    return data_.size() - removed_ids_.size();
>   }
>
>   // It is safe to remove elements from the map during iteration. All
> iterators
> Index: base/id_map_unittest.cc
> diff --git a/base/id_map_unittest.cc b/base/id_map_unittest.cc
> index
>
58abf9605ed46393e2fc18cc9393c3fc347df71d..54475b678919c968a2dca5db28170711d7ef7ac4
> 100644
> --- a/base/id_map_unittest.cc
> +++ b/base/id_map_unittest.cc
> @@ -67,10 +67,21 @@ TEST_F(IDMapTest,
> IteratorRemainsValidWhenRemovingCurrentElement) {
>   map.Add(&obj2);
>   map.Add(&obj3);
>
> -  for (IDMap<TestObject>::const_iterator iter(&map);
> -       !iter.IsAtEnd(); iter.Advance()) {
> -    map.Remove(iter.GetCurrentKey());
> +  {
> +    IDMap<TestObject>::const_iterator iter(&map);
> +    while (!iter.IsAtEnd()) {
> +      map.Remove(iter.GetCurrentKey());
> +      iter.Advance();
> +    }
> +
> +    // Test that while an iterator is still in scope, we get the map
> emptiness
> +    // right (http://crbug.com/35571).
> +    EXPECT_TRUE(map.IsEmpty());
> +    EXPECT_EQ(0U, map.size());
>   }
> +
> +  EXPECT_TRUE(map.IsEmpty());
> +  EXPECT_EQ(0U, map.size());
>  }
>
>  TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingOtherElements) {
>
>
>

[Paweł Hajdan Jr., 2010-02-14 18:33:25]

On 2010/02/14 18:13:03, brettw wrote:
> LGTM. Is IsEmpty called during normal running? (I'm at home so can't
> easily check). If it is used for something useful, I'd say land it on
> the branch. If it's just this test, trunk should be fine.

See the bug for more info. This was causing a RenderProcessHost leak, and
possibly other problems (I remember IDMap iterators being used in few other
places).

[brettw, 2010-02-15 04:14:54]

It just wasn't clear to me if this is just a leak in a test or happens
when browsing. It looks low risk enough in any case that we should ask
to land on the branch if that's still possible.

Brett

On Sun, Feb 14, 2010 at 10:33 AM,  <phajdan.jr@chromium.org> wrote:
> On 2010/02/14 18:13:03, brettw wrote:
>>
>> LGTM. Is IsEmpty called during normal running? (I'm at home so can't
>> easily check). If it is used for something useful, I'd say land it on
>> the branch. If it's just this test, trunk should be fine.
>
> See the bug for more info. This was causing a RenderProcessHost leak, and
> possibly other problems (I remember IDMap iterators being used in few other
> places).
>
> http://codereview.chromium.org/604048
>