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


Submitter:	Xiaohong Bao	Reviewers
Branch:	https://bitbucket.org/BaoLinden/viewer-development-storm-973	Groups:	viewer
Bugs:	storm-973	People:
Change Number:	None	Repository:	viewer-development

Description:

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.

Testing Done:

Expand All

A description would not hurt:
* What caused the bug?
* What fixes it?

Vadim ProductEngine April 8, 2011, 10:09 a.m. (April 8, 2011, 10:09 a.m.)

Ah, and the fix fails to apply cleanly to viewer-development AND to BaoLinden/viewer-development-storm-973.

Description

Changed from:

this is to resubmit the patch for storm-973.

Changed to:

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.

Description:

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.

Vadim ProductEngine April 8, 2011, 1:01 p.m. (April 8, 2011, 1:01 p.m.)

> 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. 
Manual merge with what? I just applied the patch to the revision it was made for. There were no conflicting changes.
Let me elaborate...
The patch claims to have been generated on top of changeset 13670741a0a8, which doesn't have the "if(gNoRender) return;" line in LLViewerTextureList::decodeAllImages(), thus the patch fails to apply.
I'm pretty sure there's something wrong with the way the patch was generated.

BTW, viewer-development and viewer-development-storm-973 are absolutely identical. What's the point of having an identical clone?

Vadim ProductEngine April 8, 2011, 1:03 p.m. (April 8, 2011, 1:03 p.m.)

I was wrong. The patch was made for changeset 8395569a8f77, which is missing in both repositories. That must be the reason...

Vadim ProductEngine April 8, 2011, 2:11 p.m. (April 8, 2011, 2:11 p.m.)

Oops. The first comment is correct. The patch indeed was made for 13670741a0a8.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

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 April 14, 2011, 3:55 p.m. (April 14, 2011, 3:55 p.m.)

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 April 14, 2011, 4:21 p.m. (April 14, 2011, 4:21 p.m.)

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

Xiaohong Bao April 14, 2011, 6:17 p.m. (April 14, 2011, 6:17 p.m.)

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

indra/newview/llviewertexturelist.cpp (Diff revision 1)

ditto

indra/newview/llviewertexturelist.cpp (Diff revision 1)

* 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 April 14, 2011, 3:57 p.m. (April 14, 2011, 3:57 p.m.)

1, no need to check mInitialized there.
2, to avoid calling LLThread::currentID() because that piece of code is executed very frequently.

Vadim ProductEngine April 14, 2011, 4:35 p.m. (April 14, 2011, 4:35 p.m.)

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.

Xiaohong Bao April 14, 2011, 6:15 p.m. (April 14, 2011, 6:15 p.m.)
```
right, that piece is only called in the main thread. 
```

I guess there is no real reason why there's a space before some semicolons but not others, or is there? Even although I like to look with the space there, I think we should remove those spaces to be consistent with common practice.

indra/newview/llviewertexturelist.h (Diff revision 1)

Any reason for this to be BOOL instead of bool?
Also, remove the space between the name and the semicolon.

Xiaohong Bao April 14, 2011, 3:58 p.m. (April 14, 2011, 3:58 p.m.)
```
BOOL is one byte, easy for memory alignment.
```

Vadim ProductEngine April 14, 2011, 4:38 p.m. (April 14, 2011, 4:38 p.m.)

Per coding standard:
* "Prefer the use of the standard C++ bool."
* "BOOL denotes a 32-bit integer value <...>"

indra/newview/llviewertexturelist.cpp (Diff revision 1)

Remove space between value and semicolon.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

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().)

Xiaohong Bao April 14, 2011, 3:59 p.m. (April 14, 2011, 3:59 p.m.)

does not need to because that function is called only in the main thread.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

Remove space before semicolon.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

Same here.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

And while we're at it, here, too.

indra/newview/llviewertexturelist.cpp (Diff revision 1)

And there's more of these spaces. (I won't comment on all.)

I assume you'll add the comment before committing this.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.