Crash in LLRemoteParcelInfoProcessor::processParcelInfoReply()
Review Request #24 - Created Dec. 15, 2010 and submitted
| Kitty Barnett | Reviewers | ||
| viewer | |||
| VWR-24207 | |||
| None | viewer-development | ||
erase() on a multimap will only invalidate iterators that point to the element being erased so pre-incrementing the loop iterator should prevent it from getting invalidated when an observer calls removeObserver() as part of its processParcelInfo() implementation.
Expand AllPosted (Dec. 16, 2010, 6:53 a.m.)
-
indra/newview/llremoteparcelrequest.cpp (Diff revision 1) -
post-increments are subtle ... please include some comment in the code that points out what's going on. (Can be short)
-
indra/newview/llremoteparcelrequest.cpp (Diff revision 1) -
Please also comment in the code why cur_oi may not be oi when calling this, so we don't risk someone reverting your fix by accident.
Posted (Dec. 16, 2010, 8:29 a.m.)
Putting the "oi++" in the same line as the affectation is unclear. Please put that on the next line and write a comment as to why this increment must happen as soon as the affectation to cur_oi is done (the whole point of the patch). I'm afraid that someone reviewing casually the code would "simplify" this by putting the increment back in the "for" statement.
Wow, nice catch. I was looking for this bug earlier and didn't dream that a function like processParcelInfo would attempt to erase an entity.
Review request changed
Updated (Dec. 16, 2010, 11:06 a.m.)
-
- added Diff r2
Added comments.
I'm wondering if there is still a reason for the deadlist_t related code now?
If that was "inlined" into the loop it would become:
while (oi != end)
{
// increment the loop iterator now since it may become invalid below
observer_multimap_t::iterator cur_oi = oi++;
LLRemoteParcelInfoObserver * observer = cur_oi->second.get();
if(observer)
{
// may invalidate cur_oi if the observer removes itself
observer->processParcelInfo(parcel_data);
}
else
{
// the handle points to an expired observer, so don't keep it
// around anymore (invalidates cur_oi)
observers.erase(cur_oi);
}
}
That might decrease the chance of the fix being reverted since even if the comment on "processParcelInfo" is missed, then "observers.erase()" would still make the need for two iterators ("oi" and "cur_oi") clear?
Other reviews