314109 - Changing the email regexp of a group causes the next flag-only change to crash

Status: RESOLVED WONTFIX

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Dave Miller [:justdave]]

A user emailed me the following:

-----8<-----
Bugzilla has suffered an internal error. Please save this page and send it to [e-mail] with details of what you were doing at the time this message appeared.

URL: https://bugzilla.mozilla.org/process_bug.cgi
Attempted to lock a table without releasing previous lock first:

Tables already locked:
bugs WRITE, bugs_activity WRITE, cc WRITE, cc AS selectVisible_cc WRITE, profiles WRITE, dependencies WRITE, votes WRITE, products READ, components READ, keywords WRITE, longdescs WRITE, fielddefs WRITE, bug_group_map WRITE, flags WRITE, duplicates WRITE, user_group_map WRITE, group_group_map READ, flagtypes READ, flaginclusions AS i READ, flagexclusions AS e READ, keyworddefs READ, groups READ, attachments READ, group_control_map AS oldcontrolmap READ, group_control_map AS newcontrolmap READ, group_control_map READ, email_setting READ

Tables requesting locking:
profiles WRITE, user_group_map WRITE, group_group_map READ, groups READ 
-----8<-----

He includes the screenshot of the midair he got when he attempted to re-submit it, which shows that the only changes we was making was adding a comment and CCing himself.

The timestamp of his email roughly coincides with when I changed the emailregexp on a group earlier today, and I'm told the "Tables requesting locking" list given in the error belongs to the derive_groups() method, which makes that a quite likely suspect.

Comment 1

[Joel Peshkin]

wow...

I'll look later, but I have a suspicion.

It used to be true that, if a group had changed since it was rederived, then it had to be rederived (regexp and all) when the user next had a user object created.  Now that we keep the group regexps up-to-date, it is no longer necessary to do that and we handle it by flattenng (just reads) instead.

I am guessing that there is still some code that sees the change and obtains a lock.  Since it no longer needs to do anything during that lock, it might then forget to unlock it as well.  So... it would do something like this.

I'll try to look tonight.

Comment 2

[Frédéric Buclin]

joel, don't forget that you removed User::derive_groups() in 2.21 only, not 2.20, see bug 304583. I'm not sure you fixed the regexp problem in 2.20.

Comment 3

[Joel Peshkin]

Right.  This is unique to 2.20

This is something of a long shot, but it is possible that there is a race where the user began process_bug before you changed the regexp.  This means that the user object creation by Bugzilla->login would not have forced a group rederivation.  After that, but before the lock (during data validation), you changed the regexp.  So, when process_bug obtained the lock, the groups for the user were in need of derivation.  Then, some library routine, would have to create a new user object rather than using one created before the lock and BAM!

I looked through all the functions that happened during the lock and did not find anything that appears to do this.  One thing we could do to protect against this would be to force the creation of a new user object for the current user immediately after the lock and ensure that that creation is accompanied by the "already_locked" parameter.  That would make sure that this cannot happen.

I dont seeany harm in doing that, but I have not isolated this problem well enough to be confident that it fixes it either.

The other problem I have with this theory is the fact that the user midaired when he resubmitted.  That means that he must have made it to this code....

    if ($bug_changed) {
        SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp WHERE bug_id = $id");
    }
    $dbh->bz_unlock_tables();

On which bug did this occur and at what time?  Is it possible a flag was changed?

Comment 4

[Lewis Rosenthal]

(In reply to comment #3)
> On which bug did this occur and at what time?  Is it possible a flag was
> changed?
> 
(I was the submitter.) It was bug 104989 comment #44.

I did indeed log in and start my composition of my comment earlier in the day. I was then interrupted, left for lunch, and finished up a couple hours later.

Lewis

Comment 5

[Joel Peshkin]

Well, I still have a bit of trouble explining this properly.   The midair shows that we got far enough to bump delta_ts on the original attempt, yet the tables are still locked. What am I missing here?

Comment 6

[Frédéric Buclin]

Lewis, did you try to change/set/cancel a flag?

The only reason I could see this error is when a flag changes and Flag.pm tries to check the requester/requestee privs... the error cannot happen when trying to notify someone, because Flag::notify calls "new Bugzilla::User" with DERIVE_GROUPS_TABLES_ALREADY_LOCKED, so it doesn't try to lock tables.

justdave, I agree with joel, this is probably a race condition and is hard to reproduce. I don't see this bug as a blocker as the probability to reproduce it again is very low.

Comment 7

[Max Kanat-Alexander]

Okay, given our new information about the bug (it's uncommon and difficult to reproduce), I think that this bug is not a blocker. If anybody disagrees, they can ask justdave to override me.

Comment 8

[Frédéric Buclin]

Probably related to this bug:
http://landfill.bugzilla.org/selenium/TestRunner.html?test=bugzilla/2.20/BugzillaTestSuite.html

Run the "Test User Group (*)" test (you have to be logged in on qa220 with editusers/admin privs first).

Comment 9

[Frédéric Buclin]

per comment 8

Comment 10

[Max Kanat-Alexander]

Sure, this seems like a decent thing to fix for 2.20.2.

Comment 11

[Max Kanat-Alexander]

I don't think that this is actually a significant-enough blocker to stop any release. It's not as though it's being constantly reported; it just causes some trouble with our QA Tests. However, it hasn't been important enough for anybody to assign it to themselves, so it doesn't look like it's getting fixed today.

Comment 12

[Frédéric Buclin]

Bugzilla 2.20 is restricted to security bugs only and this bug is very hard to reproduce anyway. Instead of taking the risk to break the branch, I much prefer to leave it unfixed, especially because newer releases are not affected.

Comment 13

[Frédéric Buclin]

*** Bug 353568 has been marked as a duplicate of this bug. ***