STORM-1458 ([crashhunters] crash at LLParticipantList::LLParticipantListMenu::isGroupModerator() [secondlife-bin llparticipantlist.cpp])


Submitter:	Paul ProductEngine	Reviewers
Branch:		Groups:	viewer
Bugs:	storm-1458	People:
Change Number:	None	Repository:	viewer-development

Description:

- Added null checking

Testing Done:

Expand All

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

Either this comment should be moved inside the if-block or it should be formulated as a question:
// Is agent in group call?

Otherwise it might give the wrong impression that it is already known that the agent is in a group call at the point where the comment appears.

Also, is this really about group calls (i.e. voice) or rather/also group chat (i.e. text).

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

I think if the speaker manager is missing (pointer is null), at least a warning in the log would be warranted.

I'm not sure whether the same applies for missing (i.e. null) speaker. (Could the agent have left the group call/chat meanwhile thus not be in the speaker list anymore?)

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

This comment should read
		// Is agent a moderator?
or similar.

Diff
- added Diff r2

Description:

Updated diff according to Boroondas' commentaries

indra/newview/llparticipantlist.cpp (Diff revision 2)

Hmm ... looking at the comment at https://bitbucket.org/lindenlab/viewer-development/src/d8c37b383028/indra/newview/llagent.h#cl-705 and the implementation of isInGroup at https://bitbucket.org/lindenlab/viewer-development/src/d8c37b383028/indra/newview/llagent.cpp#cl-2414 , this test actually checks whether the agent in question is a member of the group which has an UUID matching the conversation's session ID.

Because the session IDs of group chats will match the UUID of the respective groups (and all other chat session types should have session IDs not matching any group UUIDs), this also tells us whether the session is a group chat.

So maybe the comment should be yet different to reflect this. (Sorry, should have looked up this stuff for the first review already.) Maybe:
	// Is session a group chat? (It is if the session ID is the UUID of a group of which the agent is a member.)

I wonder whether there is a better way (as in: making less assumptions / not using an agent as pivot) to test for this.

Paul ProductEngine July 21, 2011, 5:26 a.m. (July 21, 2011, 5:26 a.m.)
```
I think the comment may be: "Is agent in group call/chat ?"
```

Boroondas Gupte July 21, 2011, 6:14 a.m. (July 21, 2011, 6:14 a.m.)

We don't really care here whether the *agent* is in a group call/chat, but whether the *session* here is that of a *group* call/chat (as opposed to resident-to-resident chat and ad-hoc chat sessions). We just (ab)use the agent to get a list of candidate groups one of which might have the session ID as UUID. At least, that's what the code here seems to do. If the intention is something else, we might have a bug here.

Though it would make sense that this be in fact the intention, as the notion of "moderators" does not exist in ad-hoc and resi2resi chat sessions.


> I wonder whether there is a better way (as in: making less assumptions / not using an agent as pivot) to test for this.

It looks like there isn't a better way, currently: LLIMModel::LLIMSession keeps track of the session type and offers this information through e.g. bool isGroupSessionType(), but LLParticipantList::LLParticipantListMenu seems neither directly nor indirectly (e.g. via mParent.mSpeakerMgr) aware of the LLIMSession it belongs to, so can't query that.

Off course, some refactoring could solve that, but that's not in-scope for this crash fix.

Diff
- added Diff r3

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.