Bug 277782: _throw_error should unlock tables when tables are locked, automatically

Bugzilla/Auth/Login/WWW/CGI.pm

@@ -184,7 +184,7 @@ sub login {
 
     # If we get here, then we've run out of options, which shouldn't happen
     ThrowCodeError("authres_unhandled", { authres => $authres, 
-                                          type => $type, });
+                                          type => $type });
 }
 
 # This auth style allows the user to log out.

Bugzilla/Bug.pm

@@ -511,21 +511,18 @@ sub ValidateTime {
     # (allow negatives, though, so people can back out errors in time reporting)
     if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) {
         ThrowUserError("number_not_numeric",
-                       {field => "$field", num => "$time"},
-                       "abort");
+                       {field => "$field", num => "$time"});
     }
 
     # Only the "work_time" field is allowed to contain a negative value.
     if ( ($time < 0) && ($field ne "work_time") ) {
         ThrowUserError("number_too_small",
-                       {field => "$field", num => "$time", min_num => "0"},
-                       "abort");
+                       {field => "$field", num => "$time", min_num => "0"});
     }
 
     if ($time > 99999.99) {
         ThrowUserError("number_too_large",
-                       {field => "$field", num => "$time", max_num => "99999.99"},
-                       "abort");
+                       {field => "$field", num => "$time", max_num => "99999.99"});
     }
 }
 

Bugzilla/Error.pm

@@ -32,13 +32,15 @@ use Bugzilla::Util;
 use Date::Format;
 
 sub _throw_error {
-    my ($name, $error, $vars, $unlock_tables) = @_;
+    my ($name, $error, $vars) = @_;
 
     $vars ||= {};
 
     $vars->{error} = $error;
 
-    Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT) if $unlock_tables;
+    # Make sure any locked tables are unlocked
+    # and the transaction is rolled back (if supported)
+    Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT);
 
     # If a writable data/errorlog exists, log error details there.
     if (-w "data/errorlog") {
@@ -95,6 +97,10 @@ sub ThrowCodeError {
 sub ThrowTemplateError {
     my ($template_err) = @_;
 
+    # Make sure any locked tables are unlocked
+    # and the transaction is rolled back (if supported)
+    Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT);
+
     my $vars = {};
     if (Bugzilla->batch) {
         die("error: template error: $template_err");
@@ -149,16 +155,16 @@ Bugzilla::Error - Error handling utilities for Bugzilla
   ThrowUserError("error_tag",
                  { foo => 'bar' });
  
-  # supplying "abort" to ensure tables are unlocked
-  ThrowUserError("another_error_tag",
-                 { foo => 'bar' }, 'abort');
-
 =head1 DESCRIPTION
 
 Various places throughout the Bugzilla codebase need to report errors to the
 user. The C<Throw*Error> family of functions allow this to be done in a
 generic and localisable manner.
 
+These functions automatically unlock the database tables, if there were any
+locked. They will also roll back the transaction, if it is supported by
+the underlying DB.
+
 =head1 FUNCTIONS
 
 =over 4
@@ -170,12 +176,6 @@ of variables as a second argument. These are used by the
 I<global/user-error.html.tmpl> template to format the error, using the passed
 in variables as required.
 
-An optional third argument may be supplied. If present, the error
-handling code will unlock the database tables: it is a Bugzilla standard
-to provide the string "abort" as the argument value. In the long term,
-this argument will go away, to be replaced by transactional C<rollback>
-calls. There is no timeframe for doing so, however.
-
 =item C<ThrowCodeError>
 
 This function is used when an internal check detects an error of some sort.

CGI.pl

@@ -112,7 +112,7 @@ sub CheckFormField (\%$;\@) {
             $field = $fieldname;
         }
         
-        ThrowCodeError("illegal_field", { field => $field }, "abort");
+        ThrowCodeError("illegal_field", { field => $field });
       }
 }
 
@@ -213,7 +213,7 @@ sub CheckEmailSyntax {
     my ($addr) = (@_);
     my $match = Param('emailregexp');
     if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) {
-        ThrowUserError("illegal_email_address", { addr => $addr }, 'abort');
+        ThrowUserError("illegal_email_address", { addr => $addr });
     }
 }
 

editclassifications.cgi

@@ -300,12 +300,10 @@ if ($action eq 'update') {
 
     if ($classification ne $classificationold) {
         unless ($classification) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError("classification_not_specified")
         }
         
         if (TestClassification($classification)) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError("classification_already_exists", { name => $classification });
         }
         $sth = $dbh->prepare("UPDATE classifications

editcomponents.cgi

@@ -66,7 +66,7 @@ sub CheckProduct ($)
 
     # do we have a product?
     unless ($prod) {
-        ThrowUserError('product_not_specified');    
+        ThrowUserError('product_not_specified');
         exit;
     }
 
@@ -585,7 +585,6 @@ if ($action eq 'update') {
 
     if ($description ne $descriptionold) {
         unless ($description) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('component_blank_description',
                            {'name' => $componentold});
             exit;
@@ -603,7 +602,6 @@ if ($action eq 'update') {
 
         my $initialownerid = login_to_id($initialowner);
         unless ($initialownerid) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('component_need_valid_initialowner',
                            {'name' => $componentold});
             exit;
@@ -621,7 +619,6 @@ if ($action eq 'update') {
     if (Param('useqacontact') && $initialqacontact ne $initialqacontactold) {
         my $initialqacontactid = login_to_id($initialqacontact);
         if (!$initialqacontactid && $initialqacontact ne '') {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('component_need_valid_initialqacontact',
                            {'name' => $componentold});
             exit;
@@ -638,13 +635,11 @@ if ($action eq 'update') {
 
     if ($component ne $componentold) {
         unless ($component) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('component_must_have_a_name',
                            {'name' => $componentold});
             exit;
         }
         if (TestComponent($product, $component)) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('component_already_exists',
                            {'name' => $component});
             exit;

editmilestones.cgi

@@ -59,14 +59,14 @@ sub CheckProduct ($)
 
     # do we have a product?
     unless ($product) {
-        &::ThrowUserError('product_not_specified');    
+        ThrowUserError('product_not_specified');    
         exit;
     }
 
     # Does it exist in the DB?
     unless (TestProduct $product) {
-        &::ThrowUserError('product_doesnt_exist',
-                          {'product' => $product});
+        ThrowUserError('product_doesnt_exist',
+                       {'product' => $product});
         exit;
     }
 }
@@ -506,12 +506,9 @@ if ($action eq 'update') {
     my $stored_sortkey = $sortkey;
     if ($sortkey != $sortkeyold) {
         if (!detaint_natural($sortkey)) {
-
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('milestone_sortkey_invalid',
                            {'name' => $milestone,
                             'sortkey' => $stored_sortkey});
-
             exit;
         }
 
@@ -532,12 +529,10 @@ if ($action eq 'update') {
 
     if ($milestone ne $milestoneold) {
         unless ($milestone) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('milestone_blank_name');
             exit;
         }
         if (TestMilestone($product, $milestone)) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('milestone_already_exists',
                            {'name' => $milestone,
                             'product' => $product});

editusers.cgi

@@ -212,8 +212,7 @@ if ($action eq 'search') {
     canSeeUser($otherUserID)
         || ThrowUserError('auth_failure', {reason => "not_visible",
                                            action => "modify",
-                                           object => "user"},
-                          'abort');
+                                           object => "user"});
 
     # Cleanups
     my $login           = trim($cgi->param('login')           || '');
@@ -231,11 +230,10 @@ if ($action eq 'search') {
 
         if ($login ne $loginold) {
             # Validate, then trick_taint.
-            $login || ThrowUserError('user_login_required', undef, 'abort');
+            $login || ThrowUserError('user_login_required');
             CheckEmailSyntax($login);
             is_available_username($login) || ThrowUserError('account_exists',
-                                                            {'email' => $login},
-                                                             'abort');
+                                                            {'email' => $login});
             trick_taint($login);
             push(@changedFields, 'login_name');
             push(@values, $login);
@@ -493,19 +491,17 @@ if ($action eq 'search') {
                          'whine_events WRITE');
 
     Param('allowuserdeletion')
-        || ThrowUserError('users_deletion_disabled', undef, 'abort');
+        || ThrowUserError('users_deletion_disabled');
     $editusers || ThrowUserError('auth_failure',
                                  {group  => "editusers",
                                   action => "delete",
-                                  object => "users"},
-                                 'abort');
+                                  object => "users"});
     canSeeUser($otherUserID) || ThrowUserError('auth_failure',
                                                {reason => "not_visible",
                                                 action => "delete",
-                                                object => "user"},
-                                               'abort');
+                                                object => "user"});
     productResponsibilities($otherUserID)
-        && ThrowUserError('user_has_responsibility', undef, 'abort');
+        && ThrowUserError('user_has_responsibility');
 
     Bugzilla->logout_user_by_id($otherUserID);
 

editversions.cgi

@@ -406,16 +406,13 @@ if ($action eq 'update') {
 
     if ($version ne $versionold) {
         unless ($version) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('version_blank_name');
             exit;
         }
         if (TestVersion($product,$version)) {
-            $dbh->bz_unlock_tables(UNLOCK_ABORT);
             ThrowUserError('version_already_exists',
                            {'name' => $version,
                             'product' => $product});
-
             exit;
         }
         SendSQL("UPDATE bugs

globals.pl

@@ -613,11 +613,11 @@ sub ValidatePassword {
     my ($password, $matchpassword) = @_;
     
     if (length($password) < 3) {
-        ThrowUserError("password_too_short", undef, 'abort');
+        ThrowUserError("password_too_short");
     } elsif (length($password) > 16) {
-        ThrowUserError("password_too_long", undef, 'abort');
+        ThrowUserError("password_too_long");
     } elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
-        ThrowUserError("passwords_dont_match", undef, 'abort');
+        ThrowUserError("passwords_dont_match");
     }
 }
 
@@ -649,8 +649,7 @@ sub DBNameToIdAndCheck {
         return $result;
     }
 
-    ThrowUserError("invalid_username",
-                   { name => $name }, "abort");
+    ThrowUserError("invalid_username", { name => $name });
 }
 
 sub get_classification_id {

post_bug.cgi

@@ -318,8 +318,7 @@ if (UserInGroup("editbugs") && defined($::FORM{'dependson'})) {
                 }
 
                 ThrowUserError("dependency_loop_multi",
-                               { both => $both },
-                               "abort");
+                               { both => $both });
             }
         }
         my $tmp = $me;
@@ -378,14 +377,14 @@ foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) {
     if ($::FORM{$b}) {
         my $v = substr($b, 4);
         $v =~ /^(\d+)$/
-          || ThrowCodeError("group_id_invalid", undef, "abort");
+          || ThrowCodeError("group_id_invalid");
         if (!GroupIsActive($v)) {
             # Prevent the user from adding the bug to an inactive group.
             # Should only happen if there is a bug in Bugzilla or the user
             # hacked the "enter bug" form since otherwise the UI 
             # for adding the bug to the group won't appear on that form.
             $vars->{'bit'} = $v;
-            ThrowCodeError("inactive_group", undef, "abort");
+            ThrowCodeError("inactive_group");
         }
         SendSQL("SELECT user_id FROM user_group_map 
                  WHERE user_id = $::userid

process_bug.cgi

@@ -109,7 +109,7 @@ foreach my $field ("estimated_time", "work_time", "remaining_time") {
 if (UserInGroup(Param('timetrackinggroup'))) {
     my $wk_time = $::FORM{'work_time'};
     if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) {
-        ThrowUserError('comment_required', undef, "abort");
+        ThrowUserError('comment_required');
     }
 }
 
@@ -241,7 +241,7 @@ if ((($::FORM{'id'} && $::FORM{'product'} ne $::oldproduct)
         $vars->{'newvalue'} = $::FORM{'product'};
         $vars->{'field'} = 'product';
         $vars->{'privs'} = $PrivilegesRequired;
-        ThrowUserError("illegal_change", $vars, "abort");
+        ThrowUserError("illegal_change", $vars);
     }
 
     CheckFormField(\%::FORM, 'product', \@::legal_product);
@@ -1232,7 +1232,7 @@ foreach my $id (@idlist) {
                 $vars->{'field'} = $col;
             }
             $vars->{'privs'} = $PrivilegesRequired;
-            ThrowUserError("illegal_change", $vars, "abort");
+            ThrowUserError("illegal_change", $vars);
         }
     }
 
@@ -1251,13 +1251,13 @@ foreach my $id (@idlist) {
         $vars->{'newvalue'} = "no keywords";
         $vars->{'field'} = "keywords";
         $vars->{'privs'} = $PrivilegesRequired;
-        ThrowUserError("illegal_change", $vars, "abort");
+        ThrowUserError("illegal_change", $vars);
     }
 
     $oldhash{'product'} = get_product_name($oldhash{'product_id'});
     if (!CanEditProductId($oldhash{'product_id'})) {
         ThrowUserError("product_edit_denied",
-                      { product => $oldhash{'product'} }, "abort");
+                      { product => $oldhash{'product'} });
     }
 
     if (defined $::FORM{'product'} 
@@ -1265,7 +1265,7 @@ foreach my $id (@idlist) {
         && $::FORM{'product'} ne $oldhash{'product'}
         && !CanEnterProduct($::FORM{'product'})) {
         ThrowUserError("entry_access_denied",
-                       { product => $::FORM{'product'} }, "abort");
+                       { product => $::FORM{'product'} });
     }
     if ($requiremilestone) {
         # musthavemilestoneonaccept applies only if at least two
@@ -1283,7 +1283,7 @@ foreach my $id (@idlist) {
             # if musthavemilestoneonaccept == 1, then the target
             # milestone must be different from the default one.
             if ($value eq $defaultmilestone) {
-                ThrowUserError("milestone_required", { bug_id => $id }, "abort");
+                ThrowUserError("milestone_required", { bug_id => $id });
             }
         }
     }   
@@ -1319,7 +1319,7 @@ foreach my $id (@idlist) {
                 next if $i eq "";
                 
                 if ($id eq $i) {
-                    ThrowUserError("dependency_loop_single", undef, "abort");
+                    ThrowUserError("dependency_loop_single");
                 }
                 if (!exists $seen{$i}) {
                     push(@{$deptree{$target}}, $i);
@@ -1363,8 +1363,7 @@ foreach my $id (@idlist) {
                     }
 
                     ThrowUserError("dependency_loop_multi",
-                                   { both => $both },
-                                   "abort");
+                                   { both => $both });
                 }
             }
             my $tmp = $me;
@@ -1573,6 +1572,7 @@ foreach my $id (@idlist) {
                     shift @oldlist;
                 } else {
                     if ($oldlist[0] != $newlist[0]) {
+                        $dbh->bz_unlock_tables(UNLOCK_ABORT);
                         die "Error in list comparing code";
                     }
                     shift @oldlist;