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

[Vadim ProductEngine]

> 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.
> 
> Xiaohong Bao wrote:
>     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.

1) According to my experience, the crash may not happen immediately. So if you want to catch it early you should use llassert_always().


> 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?
> 
> Xiaohong Bao wrote:
>     1, no need to check mInitialized there.
>     2, to avoid calling LLThread::currentID() because that piece of code is executed very frequently.

2. Then please add a comment stating that the code is guaranteed to execute within the render thread.
   Otherwise it looks like you're hacking your own code, bypassing the (sRenderThreadID == thread_id) assertion.


- Vadim


-----------------------------------------------------------
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/eb4a84cb/attachment-0001.htm