[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 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.
> 
> Vadim ProductEngine wrote:
>     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.

right, that piece is only called in the main thread. 


> 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.
> 
> Vadim ProductEngine wrote:
>     1) According to my experience, the crash may not happen immediately. So if you want to catch it early you should use llassert_always().

it is safe to ignore that null check because image is LLPointer protected.


- 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/20110415/2a3e44fd/attachment-0001.htm