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

RESOLVED FIXED in Bugzilla 3.0

Status

()

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({selenium})

Bugzilla 3.0
selenium
Bug Flags:
approval +
approval3.0 +
blocking3.0.1 -
testcase +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.
Flags: blocking3.0.1?
(Assignee)

Comment 1

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

Comment 2

11 years ago
... and in Bugzilla::Attachment::insert_attachment_for_bug().

Comment 3

11 years ago
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.
Severity: major → normal
Flags: blocking3.0.1? → blocking3.0.1-
Priority: -- → P1
(Assignee)

Comment 4

11 years ago
(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?
(Assignee)

Comment 5

11 years ago
Created attachment 275473 [details] [diff] [review]
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.
Attachment #275473 - Flags: review?(mkanat)
(Assignee)

Comment 6

11 years ago
Created attachment 275474 [details] [diff] [review]
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

11 years ago
(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

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

Looks good to me.
Attachment #275473 - Flags: review?(mkanat) → review+
(Assignee)

Comment 9

11 years ago
Max, what's the risk of regression with such a patch?
Flags: approval?
Flags: approval3.0?

Comment 10

11 years ago
(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.
Flags: approval?
Flags: approval3.0?
Flags: approval3.0+
Flags: approval+
(Assignee)

Comment 11

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Flags: testcase+
Keywords: selenium
(Assignee)

Comment 12

11 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

11 years ago
Created attachment 276983 [details] [diff] [review]
ugly workaround

Do we want to take this ugly workaround for now, or back out the original patch?
Attachment #276983 - Flags: review?(mkanat)
(Assignee)

Comment 14

11 years ago
Created attachment 276988 [details] [diff] [review]
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.
Attachment #276983 - Attachment is obsolete: true
Attachment #276988 - Flags: review?(justdave)
Attachment #276983 - Flags: review?(mkanat)
(Assignee)

Comment 15

11 years ago
Created attachment 276994 [details] [diff] [review]
much better fix, v2

Ignore ModPerl calls.
Attachment #276988 - Attachment is obsolete: true
Attachment #276994 - Flags: review?(justdave)
Attachment #276988 - Flags: review?(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).
Attachment #276988 - Attachment is obsolete: false
Comment on attachment 276988 [details] [diff] [review]
much better fix

oops, midaired... restoring the flags I nuked
Attachment #276988 - Attachment is obsolete: true
Attachment #276988 - Flags: review-
Comment on attachment 276994 [details] [diff] [review]
much better fix, v2

this is live on bmo and tested/works.
Attachment #276994 - Flags: review?(justdave) → review+
(Assignee)

Comment 19

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.