391073 - process_bug.cgi throws an error if a bug is moved to another product or component where a given flag doesn't apply as is

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P1)

Description

[Frédéric Buclin]

Assume a bug which is in ProductA has a flag set to LpSolit:review+. Now move this bug to ProductB where a flag type with the same name (i.e. "review") exists but which has another grant group set to which LpSolit doesn't belong. Bugzilla::Flag::retarget() will detect that LpSolit is not an appropriate reviewer (as it doesn't belong to the grant group) and will tell Bugzilla::Flag::process() to delete this flag. The problem is that retarget() does its checks by calling _validate() in an eval() to avoid generating an error in case an error would be thrown. But Bugzilla::Error::_throw_error() calls Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT) even within an eval(), as expected, and the subsequent call to $dbh->bz_unlock_tables() in process_bug.cgi at line 2099 (based on 3.0.1) fails as tables have already been unlocked by Bugzilla::Error::_throw_error().

So we should either change _validate() to return 0 instead of throwing an error if something wrong is detected, but only if explicitly requested by the caller, or update process_bug.cgi to use the UNLOCK_ABORT argument to not die. Now both solutions have cons:

1) Such an error has been detected because process_bug.cgi died. Using UNLOCK_TABLE would hide such problems. Moreover, unlocking tables before all the data has been updated could bring to race conditions.

2) Updating Flag::_validate() to return 0 if requested requires more changes in the Flag.pm code as _validate() currently returns undef if no problem has been found, and the caller currently never checks the returned value. But that's probably the best fix and is not too risky, even on the 3.0 branch.

So I'm going to implement 2). This won't regress anything as the caller has to explicitly pass !THROW_ERROR to avoid generating an error. And if this argument is passed, then the caller is aware that it must check the returned value as no error will be thrown.

Comment 1

[Frédéric Buclin]

Note to self: implementing 2) would also let us remove the eval() used in post_bug.cgi.

Comment 2

[Frédéric Buclin]

... and in Bugzilla::Attachment::insert_attachment_for_bug().

Comment 3

[Max Kanat-Alexander]

You could also check $^S in bz_unlock tables and not do the abort unlock.

This isn't a blocker, because the situation where you have two identically-named flags in different products/components with different grant groups, and you have a bug that already has flags set--that's not going to be a very common situation.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> You could also check $^S in bz_unlock tables and not do the abort unlock.

Sounds good. I will give it a try.

I started patching Flag.pm and basically added a |return 0 if $skip_all_errors;| before each Throw*Error(). But 1) this makes the code harder to read, and 2) we would have to do so for most validation routines assuming the caller doesn't want the validation routine to throw an error. Probably a better alternative is to use eval() and Bugzilla->error_mode(ERROR_MODE_DIE); as we do now in post_bug.cgi and Attachment.pm. This way, the validation routines can remain "clean" and readable.

mkanat, what's your opinion about this problem as a whole, i.e. not specific to Flag.pm but the way to catch errors and take the appropriate action?

Comment 5

[Frédéric Buclin]

Attachment: patch using $^S, v1

This patch is a simple fix which prevents to unlock tables if we are within an eval(). If an error is thrown by the caller, then _throw_error() will be called again with $^S = false, so tables will correctly be unlocked.

I wonder what happens exactly if we forget to set Bugzilla->error_mode(ERROR_MODE_DIE) before calling eval() as Throw*Error() wouldn't call die(). The error message would be displayed, but if we are within a LOCK TABLES, it will fail because some tables have not been locked, see bug 277782 comment 0.

Comment 6

[Frédéric Buclin]

Attachment: patch implementing SKIP_ALL_ERRORS, v1

For comparison, here is how to fix the problem using the SKIP_ALL_ERRORS flag. This allows us to remove these eval()'s, but it requires much more changes.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> I wonder what happens exactly if we forget to set
> Bugzilla->error_mode(ERROR_MODE_DIE) before calling eval()

  ThrowError calls exit() and the script would terminate even though you were in an eval.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 275473 [details] [diff] [review]
patch using $^S, v1

Looks good to me.

Comment 9

[Frédéric Buclin]

Max, what's the risk of regression with such a patch?

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Max, what's the risk of regression with such a patch?

  It's hard to say, really. But overall, I think it's more likely to *fix* hidden errors than it is to introduce them, having just looked at every place that we use ERROR_MODE_DIE in Bugzilla.

Comment 11

[Frédéric Buclin]

tip:

Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.21; previous revision: 1.20
done

3.0:

Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.19.2.2; previous revision: 1.19.2.1
done

Comment 12

[Frédéric Buclin]

I hate mod_perl! When mod_perl is enabled, $^S is *always* true, even when the code being executed is not in an eval(). b.m.o crashes everytime Throw*Error() is called, because _throw_error() no longer unlock tables. I see no workaround. Any idea?

Comment 13

[Frédéric Buclin]

Attachment: ugly workaround

Do we want to take this ugly workaround for now, or back out the original patch?

Comment 14

[Frédéric Buclin]

Attachment: much better fix

Code stolen from http://perl.apache.org/docs/general/perl_reference/perl_reference.html#Alternative_Exception_Handling_Techniques

Works fine on mod_cgi, please test it on mod_perl.

Comment 15

[Frédéric Buclin]

Attachment: much better fix, v2

Ignore ModPerl calls.

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 276988 [details] [diff] [review]
much better fix

doesn't work on mod_perl, still claims to be in an eval.  doing last if $sub =~ /^ModPerl/ inside the loop seems to solve it (as discussed on IRC).

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 276988 [details] [diff] [review]
much better fix

oops, midaired... restoring the flags I nuked

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 276994 [details] [diff] [review]
much better fix, v2

this is live on bmo and tested/works.

Comment 19

[Frédéric Buclin]

tip:

Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.22; previous revision: 1.21
done

3.0:

Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.19.2.3; previous revision: 1.19.2.2
done