[opensource-dev] Review Request: STORM-1427 Crash in world map when region name search is empty (Assert fails in LLWorldMapMessage::processMapBlockReply)

[Vadim ProductEngine]

> On Aug. 5, 2011, 2:07 p.m., Boroondas Gupte wrote:
> > Thanks for looking into this. I've stopped counting how often I've crashed due to this.
> > 
> > > Apparently, passing empty region name to the MapNameRequest makes server return
> > > empty name in MapBlockReply, which triggered the assertion.
> > 
> > Shouldn't we warn rather than assert when we receive invalid (or just useless) data from remote (whether by our own or the servers' fault)? I don't think any service should be given the power to 'shoot down' the viewer.

The assert will not be in issue for release builds.
Triggering it in debug builds help catching bugs, as in this case.
Besides, the assert was added in attempt to locate another bug (EXT-4568), so I'd just leave it.


> On Aug. 5, 2011, 2:07 p.m., Boroondas Gupte wrote:
> > indra/newview/llfloaterworldmap.cpp, lines 1225-1229
> > <http://codereview.secondlife.com/r/432/diff/1/?file=6903#file6903line1225>
> >
> >     Instead of returning early ...

There's no point in executing the rest of the method code, so we're returning.
And I'd rather add a return than increase indentation, thus making hg blame useless at finding out why the indented code was written.


> On Aug. 5, 2011, 2:07 p.m., Boroondas Gupte wrote:
> > indra/newview/llfloaterworldmap.cpp, lines 1230-1243
> > <http://codereview.secondlife.com/r/432/diff/1/?file=6903#file6903line1230>
> >
> >     ... you could wrap the remainder of the method into a conditional block.

There's no point in executing the rest of the method code, so we're returning.
And I'd rather add a return than increase indentation, thus making hg blame useless at finding out why the indented code was written.


- Vadim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/432/#review953
-----------------------------------------------------------


On Aug. 5, 2011, 1:34 p.m., Vadim ProductEngine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/432/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2011, 1:34 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Fixed a debug assertion triggered in the world map floater.
> 
> Apparently, passing empty region name to the MapNameRequest makes server return
> empty name in MapBlockReply, which triggered the assertion.
> 
> 
> Diffs
> -----
> 
>   indra/newview/llfloaterworldmap.cpp 0fd2a1181a96 
> 
> Diff: http://codereview.secondlife.com/r/432/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vadim
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110807/f59cf639/attachment-0001.htm