305126 - Fix table locks after the removal of derive_groups

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P3)

Description

[Max Kanat-Alexander]

With the check-in of bug 304583, we will no longer need to lock tables in a few
places for derive_groups. At the least, we removed
DERIVE_GROUPS_TABLES_ALREADY_LOCKED from Flag.pm and whine.pl, so those will no
longer need those table locks (at least, not WRITE locks).

Comment 1

[Frédéric Buclin]

Also add missing tables where appropriate. For instance, User::groups() now
accesses the 'group_group_map' table, which wasn't used before, breaking table
locks which involve user objects, such as votes. :(

Comment 2

[Joel Peshkin]

The other option would be to force creation of a user object to fetch the
necessary group information right then.  How often does anyone create a user
object and then not subsequently need any group information?

Comment 3

[Marc Schumann [:Wurblzap]]

Does comment 1 make this one at least critical, or is this another bug?
(http://landfill.bugzilla.org/bugzilla-tip/votes.cgi?action=show_user crashes if
you have voted on at least one bug.)

Comment 4

[Max Kanat-Alexander]

Yeah, this is a bug, per comment 3.

Comment 5

[Joel Peshkin]

Attachment: Patch - add group_group_map to locks in post_bug and votes

I checked everything that does a read lock on user_group_map to make sure it
does a read lock on group_group_map as well.

Comment 6

[Frédéric Buclin]

Comment on attachment 193782 [details] [diff] [review]
Patch - add group_group_map to locks in post_bug and votes

User::derive_groups() used the following locks:
'profiles WRITE', 'user_group_map WRITE', 'group_group_map READ', 'groups READ'

Now User::groups() uses the same tables, but all READ. So 'profiles' and
'user_group_map' should be locked as READ now, because
User::derive_regexp_groups() is the single function to write into tables and is
never called directly when creating a user object.

Comment 7

[Frédéric Buclin]

Also, please fix the comment in attachment.cgi, line 1178 about rederiving groups.

Comment 8

[Joel Peshkin]

Attachment: v2

Comment 9

[Frédéric Buclin]

Comment on attachment 193908 [details] [diff] [review]
v2

The only reason I deny review is because of the confusing comments in
process_bug.cgi, lines 623 + 1273. Also, at lines 1269 and 1278, I think the
profiles and user_group_map tables should now be locked as READ, assuming the
comment is correct.

Maybe a nit for you, but I think it's important to clarify this point. Handling
user privs is already hard enough. ;)

Comment 10

[Joel Peshkin]

Attachment: v3

Comment 11

[Frédéric Buclin]

Comment on attachment 194232 [details] [diff] [review]
v3

r=LpSolit

Comment 12

[Joel Peshkin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.95; previous revision: 1.94
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.126; previous revision: 1.125
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.281; previous revision: 1.280
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.32; previous revision: 1.31
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.15; previous revision: 1.14
done