> 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 > >
_______________________________________________ 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
