[opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7

[Vadim ProductEngine]

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



indra/newview/llviewertexturelist.cpp
<http://codereview.secondlife.com/r/252/#comment497>

    Almost every line of this method is a potential crasher.
    Are you sure we should handle errors this way?
    
    I would:
     * Add a NULL check for the "image" parameter (besides the llassert which is not evaluated in release builds).
     * Try handling the other errors without crashing, by issuing a warning and returning FALSE.
    
    Otherwise we may fix one crash and add two.



indra/newview/llviewertexturelist.cpp
<http://codereview.secondlife.com/r/252/#comment500>

    ditto



indra/newview/llviewertexturelist.cpp
<http://codereview.secondlife.com/r/252/#comment501>

    * No check for mInitialized before accessing sRenderThreadID.
    * I don't quite get what you're trying to achieve here by passing sRenderThreadID. It doesn't guarantee that the method is invoked from thread #<sRenderThreadID>, does it?


- Vadim


On April 8, 2011, 10:26 a.m., Xiaohong Bao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/252/
> -----------------------------------------------------------
> 
> (Updated April 8, 2011, 10:26 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> this is to resubmit the patch for storm-973.
> 
> We are not very clear what causes this. But this fix is targeting three most possible causes:
> 1, a texture is failed to add into mImageList but its flag is set to be successful;
> 2, a texture status is changed not from the main thread, because gTextureList is not thread-safe;
> 3, gTextureList is accessed before it is initialized.
> 
> I regenerated the viewer-development-storm-973 branch based on the latest viewer-development branch. If you still can not apply the patch directly, I am afraid you should do the manual merge. Otherwise grant me the permission, I will do it. 
> 
> 
> This addresses bug storm-973.
>     http://jira.secondlife.com/browse/storm-973
> 
> 
> Diffs
> -----
> 
>   indra/newview/lldrawpoolbump.cpp 13670741a0a8 
>   indra/newview/llviewertexturelist.h 13670741a0a8 
>   indra/newview/llviewertexturelist.cpp 13670741a0a8 
> 
> Diff: http://codereview.secondlife.com/r/252/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Xiaohong
> 
>

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