[opensource-dev] Review Request: the fix for STORM-1026: Viewer crahes while trying to reset Graphics quality
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/180/ --- Review request for Viewer. Summary --- This is for STORM-1026: Viewer crahes while trying to reset Graphics quality. Diffs - indra/llrender/llvertexbuffer.cpp 767feb16f05f Diff: http://codereview.secondlife.com/r/180/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
[opensource-dev] Review Request: fix for STORM-1046:[crashhunters] crash in LWorld::removeRegion STORM-1014: Viewer crash in LLSurface::getWaterHeight STORM-1047:[crashhunters] crash at LLViewerObject
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/189/ --- Review request for Viewer and Merov Linden. Summary --- changeset: e4c78fbe827f Merov: sorry I can not upload the diff because the parent of this changeset is still in the queue of the codereview. This addresses bugs STORM-1014, STORM-1046 and STORM-1047. http://jira.secondlife.com/browse/STORM-1014 http://jira.secondlife.com/browse/STORM-1046 http://jira.secondlife.com/browse/STORM-1047 Diffs - Diff: http://codereview.secondlife.com/r/189/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
Re: [opensource-dev] Review Request: the fix for STORM-1026: Viewer crahes while trying to reset Graphics quality
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/180/ --- (Updated March 7, 2011, 9:33 p.m.) Review request for Viewer and Merov Linden. Summary (updated) --- The changeset: 781a5ad220e3 This addresses bug STORM-1026. http://jira.secondlife.com/browse/STORM-1026 Diffs (updated) - indra/llrender/llvertexbuffer.cpp 767feb16f05f Diff: http://codereview.secondlife.com/r/180/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
[opensource-dev] Review Request: a try fix and debug code for STORM-973:[crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/202/ --- Review request for Viewer. Summary --- this is the fix including some debug code for STORM-973. The debug code will be removed after the fix is confirmed. This addresses bug STORM-973. http://jira.secondlife.com/browse/STORM-973 Diffs - indra/newview/llviewertexturelist.h 27984babd61a indra/newview/llviewertexturelist.cpp 27984babd61a Diff: http://codereview.secondlife.com/r/202/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
[opensource-dev] Review Request: partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/218/ --- Review request for Viewer. Summary --- partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions fixed: crashed caused by snapshots, loading the last bitmap, too large texture memory due to insufficient physical memory also force to rebuild VBO if the memory is about to be running out to eliminate possible memory leaking caused by GPU driver. the changeset: 30a545579ab6 This addresses bug storm-948. http://jira.secondlife.com/browse/storm-948 Diffs - indra/newview/llappviewer.h a8639217816b indra/newview/llappviewer.cpp a8639217816b indra/newview/llviewerdisplay.cpp a8639217816b indra/newview/llviewertexturelist.h a8639217816b indra/newview/llviewertexturelist.cpp a8639217816b indra/newview/llviewerwindow.cpp a8639217816b Diff: http://codereview.secondlife.com/r/218/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
Re: [opensource-dev] Review Request: partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/218/ --- (Updated March 21, 2011, 10:16 a.m.) Review request for Viewer. Summary --- partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions fixed: crashed caused by snapshots, loading the last bitmap, too large texture memory due to insufficient physical memory also force to rebuild VBO if the memory is about to be running out to eliminate possible memory leaking caused by GPU driver. the changeset: 30a545579ab6 This addresses bug storm-948. http://jira.secondlife.com/browse/storm-948 Diffs - indra/newview/llappviewer.h a8639217816b indra/newview/llappviewer.cpp a8639217816b indra/newview/llviewerdisplay.cpp a8639217816b indra/newview/llviewertexturelist.h a8639217816b indra/newview/llviewertexturelist.cpp a8639217816b indra/newview/llviewerwindow.cpp a8639217816b Diff: http://codereview.secondlife.com/r/218/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
[opensource-dev] Review Request: more fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/222/ --- Review request for Viewer. Summary --- additional fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *) This fix https://bitbucket.org/BaoLinden/viewer-development-storm-973/changeset/a4a06cbeb360 has to be merged together with https://bitbucket.org/BaoLinden/viewer-development-storm-973/changeset/247b4c659e7f This addresses bug STORM-973. http://jira.secondlife.com/browse/STORM-973 Diffs - Diff: http://codereview.secondlife.com/r/222/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
[opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/252/ --- Review request for Viewer. Summary --- this is to resubmit the patch for storm-973. 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
Re: [opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7
--- 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. Changes --- I changed the description of the review: added the possible causes of this bug. 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. Summary (updated) --- 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
[opensource-dev] Review Request: fix for ECC-49 / storm-1147: Other avatar's loose clothing\ flared cuffs look like they are tight\got no flare in v.2.4.0 and higher
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/260/ --- Review request for Viewer. Summary --- This patch is a resident posted for ECC-49. It is correct and works fine. So please merge it into viewer-development and viewer-release. This addresses bugs /, ECC-49 and storm-1147. http://jira.secondlife.com/browse// http://jira.secondlife.com/browse/ECC-49 http://jira.secondlife.com/browse/storm-1147 Diffs - indra/newview/llviewertexture.cpp c1559a7da393 Diff: http://codereview.secondlife.com/r/260/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
Re: [opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7
> 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 #, 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
Re: [opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7
> On April 8, 2011, 3:48 p.m., Boroondas Gupte wrote: > > indra/newview/llviewertexturelist.h, line 191 > > <http://codereview.secondlife.com/r/252/diff/1/?file=1406#file1406line191> > > > > Any reason for this to be BOOL instead of bool? > > Also, remove the space between the name and the semicolon. BOOL is one byte, easy for memory alignment. > On April 8, 2011, 3:48 p.m., Boroondas Gupte wrote: > > indra/newview/llviewertexturelist.cpp, lines 91-93 > > <http://codereview.secondlife.com/r/252/diff/1/?file=1407#file1407line91> > > > > I guess the intention is that this gets only executed by one single > > thread. (And probably only once?) So before setting sRenderThreadID to the > > current thread's ID, we might want to assert that it hasn't been changed > > before (i.e., is still 0). > > > > (If we want to allow to execute this assignment several times but only > > by one thread, check whether it's 0 or already has the value of > > LLThread::currentID().) does not need to because that function is called only in the main thread. - Xiaohong --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/252/#review567 --- 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
Re: [opensource-dev] Review Request: partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions
> On March 25, 2011, 6:20 p.m., Merov Linden wrote:
> > indra/newview/llappviewer.cpp, lines 1417-1420
> > <http://codereview.secondlife.com/r/218/diff/1/?file=1306#file1306line1417>
> >
> > Aren't we in danger of immediate crash when called? If so, what happens
> > if we return here? Do we let the app crash? Shouldn't we crash here
> > immediately so we can identify in the stack trace which memory allocation
> > produces the situation?
if we can avoid crash, we try to. if we can not, then viewer will crash
somewhere. This fix can not completely eliminate memory failure but try its
best to save the viewer from crash.
> On March 25, 2011, 6:20 p.m., Merov Linden wrote:
> > indra/newview/llviewerdisplay.cpp, lines 1462-1463
> > <http://codereview.secondlife.com/r/218/diff/1/?file=1307#file1307line1462>
> >
> > No clean up necessary in that catch {} block?
no, because it is at the very last stage of a normal quitting.
> On March 25, 2011, 6:20 p.m., Merov Linden wrote:
> > indra/newview/llviewertexturelist.cpp, line 79
> > <http://codereview.secondlife.com/r/218/diff/1/?file=1309#file1309line79>
> >
> > This global declaration doesn't follow naming convention. Also, it
> > shouldn't be here but as a static member of the LLViewerTextureList class.
already changed to a private static member.
- Xiaohong
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/218/#review505
---
On March 21, 2011, 10:16 a.m., Xiaohong Bao wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/218/
> ---
>
> (Updated March 21, 2011, 10:16 a.m.)
>
>
> Review request for Viewer.
>
>
> Summary
> ---
>
> partial fix for STORM-948: [crashhunters] meta-issue for bad_alloc exceptions
> fixed: crashed caused by snapshots, loading the last bitmap, too large
> texture memory due to insufficient physical memory
> also force to rebuild VBO if the memory is about to be running out to
> eliminate possible memory leaking caused by GPU driver.
>
> the changeset: 30a545579ab6
>
>
> This addresses bug storm-948.
> http://jira.secondlife.com/browse/storm-948
>
>
> Diffs
> -
>
> indra/newview/llappviewer.h a8639217816b
> indra/newview/llappviewer.cpp a8639217816b
> indra/newview/llviewerdisplay.cpp a8639217816b
> indra/newview/llviewertexturelist.h a8639217816b
> indra/newview/llviewertexturelist.cpp a8639217816b
> indra/newview/llviewerwindow.cpp a8639217816b
>
> Diff: http://codereview.secondlife.com/r/218/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
Re: [opensource-dev] Review Request: fix for STORM-973: [crashhunters] crash at LLViewerTextureList::removeImageFromList(LLViewerFetchedTexture *)7
> 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 #, 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 > > ___ 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
