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

[Xiaohong Bao]

> On April 8, 2011, 12:53 p.m., Vadim ProductEngine wrote:
> > indra/newview/llviewertexturelist.cpp, lines 502-517
> > <http://codereview.secondlife.com/r/252/diff/1/?file=1407#file1407line502>
> >
> >     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.

This is by design. The viewer should crash when those cases happen.
1) no need to do null check for image because it never happens, and if it does happen, it will immediately crash at the line "image->isInImageList".
2)again, we need the viewer to crash there to avoid harder and unpredictable behaviors later.


> On April 8, 2011, 12:53 p.m., Vadim ProductEngine wrote:
> > indra/newview/llviewertexturelist.cpp, lines 719-721
> > <http://codereview.secondlife.com/r/252/diff/1/?file=1407#file1407line719>
> >
> >     * 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?

1, no need to check mInitialized there.
2, to avoid calling LLThread::currentID() because that piece of code is executed very frequently.


- Xiaohong


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


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/20110414/2926de06/attachment-0001.htm