Fix erase method usage on STL containers in Chrome. Invoking erase on the ite...

Fix erase method usage on STL containers in Chrome. Invoking erase on the iterator renders it invalid. We were continuing to use this iterator. 

R=darin


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5531

[Peter Kasting, 2008-11-14 02:30:19]

Thanks for this fix, it should definitely help with memory corruption.

http://codereview.chromium.org/10925/diff/201/207
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/10925/diff/201/207#newcode219
Line 219: iter = search_engine_for_url.erase(iter);
This will increment to the next element, and then the loop increment will
increase it again.  You'll need to modify how the loop iterates to avoid
skipping elements.

http://codereview.chromium.org/10925/diff/201/206
File chrome/browser/renderer_security_policy.cc (right):

http://codereview.chromium.org/10925/diff/201/206#newcode116
Line 116: delete security_state;
Nit: The new code works fine, but the old code worked fine too and was shorter
and simpler.

http://codereview.chromium.org/10925/diff/201/204
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/10925/diff/201/204#newcode1932
Line 1932: // Callers can rely on the iterator being valid on return.
Nit: It might be better to simply comment the declaration of this function to
say that it can invalidate |iter| and change the contents of the class |iter|
points to, since no one seems to make use of the returned pointer, and doing so
is potentially tricky?  (I would also change |iter| to a const& in this case)

[ananta, 2008-11-14 02:43:30]

Hi Peter

Thanks for the comments. I have updated the CB. Please take another look.

Thanks
Ananta
On 2008/11/14 02:30:19, pkasting wrote:
> Thanks for this fix, it should definitely help with memory corruption.
> 
> http://codereview.chromium.org/10925/diff/201/207
> File chrome/browser/importer/firefox_importer_utils.cc (right):
> 
> http://codereview.chromium.org/10925/diff/201/207#newcode219
> Line 219: iter = search_engine_for_url.erase(iter);
> This will increment to the next element, and then the loop increment will
> increase it again.  You'll need to modify how the loop iterates to avoid
> skipping elements.
> 
> http://codereview.chromium.org/10925/diff/201/206
> File chrome/browser/renderer_security_policy.cc (right):
> 
> http://codereview.chromium.org/10925/diff/201/206#newcode116
> Line 116: delete security_state;
> Nit: The new code works fine, but the old code worked fine too and was shorter
> and simpler.
> 
> http://codereview.chromium.org/10925/diff/201/204
> File chrome/browser/resource_dispatcher_host.cc (right):
> 
> http://codereview.chromium.org/10925/diff/201/204#newcode1932
> Line 1932: // Callers can rely on the iterator being valid on return.
> Nit: It might be better to simply comment the declaration of this function to
> say that it can invalidate |iter| and change the contents of the class |iter|
> points to, since no one seems to make use of the returned pointer, and doing
so
> is potentially tricky?  (I would also change |iter| to a const& in this case)

[ananta, 2008-11-14 17:31:20]

It looks like invoking the postfix increment operator on an iterator being
erased is legit. Reverted part of the changes in the original CB. Please take
another look.

Thanks
Ananta

On 2008/11/14 02:43:30, ananta wrote:
> Hi Peter
> 
> Thanks for the comments. I have updated the CB. Please take another look.
> 
> Thanks
> Ananta
> On 2008/11/14 02:30:19, pkasting wrote:
> > Thanks for this fix, it should definitely help with memory corruption.
> > 
> > http://codereview.chromium.org/10925/diff/201/207
> > File chrome/browser/importer/firefox_importer_utils.cc (right):
> > 
> > http://codereview.chromium.org/10925/diff/201/207#newcode219
> > Line 219: iter = search_engine_for_url.erase(iter);
> > This will increment to the next element, and then the loop increment will
> > increase it again.  You'll need to modify how the loop iterates to avoid
> > skipping elements.
> > 
> > http://codereview.chromium.org/10925/diff/201/206
> > File chrome/browser/renderer_security_policy.cc (right):
> > 
> > http://codereview.chromium.org/10925/diff/201/206#newcode116
> > Line 116: delete security_state;
> > Nit: The new code works fine, but the old code worked fine too and was
shorter
> > and simpler.
> > 
> > http://codereview.chromium.org/10925/diff/201/204
> > File chrome/browser/resource_dispatcher_host.cc (right):
> > 
> > http://codereview.chromium.org/10925/diff/201/204#newcode1932
> > Line 1932: // Callers can rely on the iterator being valid on return.
> > Nit: It might be better to simply comment the declaration of this function
to
> > say that it can invalidate |iter| and change the contents of the class
|iter|
> > points to, since no one seems to make use of the returned pointer, and doing
> so
> > is potentially tricky?  (I would also change |iter| to a const& in this
case)

[darin (slow to review), 2008-11-14 18:18:22]

LGTM

[Peter Kasting, 2008-11-14 20:25:14]

LGTM with tiny nits below, but I would like you to put back your changes to
eliminate the postfix incrementing.  It's not at all obvious that that works
correctly, whereas the way you changed it to is standard STL practice and also
matches most of the rest of our code.

http://codereview.chromium.org/10925/diff/23/227
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/10925/diff/23/227#newcode198
Line 198: bool should_increment_iter = true;
Nit: "need_to_increment_iter" might be clearer.  I suggest not initializing
this.

http://codereview.chromium.org/10925/diff/23/227#newcode236
Line 236: iter++;
Nit: Use prefix increment here.

[MAD, 2009-02-26 18:13:24]

Did I miss something or... ???

BYE
MAD

http://codereview.chromium.org/10925/diff/29/31
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/10925/diff/29/31#newcode222
Line 222: need_to_increment_iter = false;
This is not the same iter as the one used for the loop...
So this fix introduced bug http://crbug.com/2569...
Could I simply revert this change? I don't think it was fixing anything here...

[Peter Kasting, 2009-02-26 18:29:44]

On 2009/02/26 18:13:24, mad wrote:
> http://codereview.chromium.org/10925/diff/29/31#newcode222
> Line 222: need_to_increment_iter = false;
> This is not the same iter as the one used for the loop...
> So this fix introduced bug http://crbug.com/2569...
> Could I simply revert this change? I don't think it was fixing anything
here...

You're right, this fix was bogus.  Heh.

Leave the other three files, and I'd revert this one.  When you do, can you also
change the inner variable to not be named the same as the outer one?  How
confusing.