> 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
