Modified extensions::StorageInfoProvider::CheckWatchedStorages() to avoid dereferencing an invalid …

Modified extensions::StorageInfoProvider::CheckWatchedStorages() to avoid dereferencing an invalid iterator.

BUG=none
TEST=none
CID=106968


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

[kmadhusu, 2013-01-04 01:03:16]

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(left):

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:110:
storage_id_to_size_map_.erase(it);
Please avoid inserting/deleting the map elements while the map is accessed in a
for loop. Because these operations invalidates the iterator and leads to an
undefined behavior.

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:118:
if (it->second != info.available_capacity) {
At (1): Condition "it != this->storage_id_to_size_map_.end()", taking true
branch
At (2): Condition "!this->QueryUnitInfo(it->first, &info)", taking true branch
At (3): "erase" invalidates iterator "it".
At (4): Condition "this->storage_id_to_size_map_.size() == 0", taking false
branch
At (5): Using invalid iterator "it".

[Hongbo Min, 2013-01-04 01:50:06]

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(left):

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:110:
storage_id_to_size_map_.erase(it);
On 2013/01/04 01:03:16, kmadhusu wrote:
> Please avoid inserting/deleting the map elements while the map is accessed in
a
> for loop. Because these operations invalidates the iterator and leads to an
> undefined behavior.

Good catch! Using a list to cache the elements to be erased generally works.
Alternatively, it might be better to use the following pattern which is a common
practice in chromium code base:

for (it = storage_id_to_size_map_.begin(); it != storage_id_to_size_map_.end();
) {
  if (!QueryUnitInfo(it->first, &info)) {
    storage_id_to_size_map_.erase(it++);
    // ...
    continue;
  } else {
    // ...
    ++it;
  }
}

So that we could avoid introducing an additional caching list.

PS...According to the C++ standard 23.1.2.8, it says:
The insert members shall not affect the validity of iterators and references to
the container, and the erase members shall invalidate only iterators and
references to the erased elements.

So erasing an element when iteration should be fine but may vary from compiler.
It shall be better to the above pattern to handle this case. See
http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-whi....

[kmadhusu, 2013-01-04 02:29:11]

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(left):

https://codereview.chromium.org/11748025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:110:
storage_id_to_size_map_.erase(it);
On 2013/01/04 01:50:06, Hongbo Min wrote:
> On 2013/01/04 01:03:16, kmadhusu wrote:
> > Please avoid inserting/deleting the map elements while the map is accessed
in
> a
> > for loop. Because these operations invalidates the iterator and leads to an
> > undefined behavior.
> 
> Good catch! Using a list to cache the elements to be erased generally works.
> Alternatively, it might be better to use the following pattern which is a
common
> practice in chromium code base:
> 
> for (it = storage_id_to_size_map_.begin(); it !=
storage_id_to_size_map_.end();
> ) {
>   if (!QueryUnitInfo(it->first, &info)) {
>     storage_id_to_size_map_.erase(it++);
>     // ...
>     continue;
>   } else {
>     // ...
>     ++it;
>   }
> }
> 
> So that we could avoid introducing an additional caching list.
> 
> PS...According to the C++ standard 23.1.2.8, it says:
> The insert members shall not affect the validity of iterators and references
to
> the container, and the erase members shall invalidate only iterators and
> references to the erased elements.
> 
> So erasing an element when iteration should be fine but may vary from
compiler.
> It shall be better to the above pattern to handle this case. See
>
http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-whi....

Sure. Done.

[Hongbo Min, 2013-01-04 02:37:49]

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:122:
if (storage_id_to_size_map_.size() == 0) {
Pls move this code block right after line 109, otherwise, it should have no
chance to get running.

[kmadhusu, 2013-01-04 02:45:18]

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:122:
if (storage_id_to_size_map_.size() == 0) {
On 2013/01/04 02:37:49, Hongbo Min wrote:
> Pls move this code block right after line 109, otherwise, it should have no
> chance to get running.

This code will run as expected. Why do you think it should be within the for
loop? To me, the new code is easier to read and understand.

[Hongbo Min, 2013-01-04 02:49:25]

lgtm! Thanks.

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/11748025/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:122:
if (storage_id_to_size_map_.size() == 0) {
On 2013/01/04 02:45:18, kmadhusu wrote:
> On 2013/01/04 02:37:49, Hongbo Min wrote:
> > Pls move this code block right after line 109, otherwise, it should have no
> > chance to get running.
> 
> This code will run as expected. Why do you think it should be within the for
> loop? To me, the new code is easier to read and understand.

You are right. I was thinking it is in for loop scope.

[commit-bot: I haz the power, 2013-01-04 02:55:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[kmadhusu, 2013-01-04 02:59:31]

benwells@: Can you please review the code?

Thanks.

[benwells, 2013-01-04 03:21:43]

lgtm

[commit-bot: I haz the power, 2013-01-04 16:46:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11748025/4001

[commit-bot: I haz the power, 2013-01-04 18:13:26]

Retried try job too often on win_rel for step(s) unit_tests

[commit-bot: I haz the power, 2013-01-04 18:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11748025/4001

[commit-bot: I haz the power, 2013-01-04 21:05:57]

Change committed as 175191