246778 - ThrowUserError causes internal error with timetracking

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Marc Schumann [:Wurblzap]]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

Similarly to the effects described in bug 240036, ThrowUserError() crashes 
with "Table 'namedqueries' was not locked with LOCK TABLES" when you enter non-
numerical values for estimated hours.

Reproducible: Always
Steps to Reproduce:
1. Log on to Bugzilla with an account that is a member of the group
   that is configured in the "timetrackinggroup" param.
2. On a bug, enter "1,5" instead of "1.5" in the "estimated hours" field.
3. Click submit.
Actual Results:  
The described Bugzilla internal error is shown.

Expected Results:  
A well-formed error message from user-error.html.tmpl should show up ("Hours 
requires a numeric value").

Equivalently to bug 240036, this can be cured by replacing
   ThrowUserError("need_numeric_value")
with
   ThrowUserError("need_numeric_value", "abort")
in globals.pl.

Perhaps there are more places in the code where a similar change may be 
necessary?

Comment 1

[Marc Schumann [:Wurblzap]]

Sorry about my confusion -- please make that "Hours Worked" where I'm talking 
about estimated hours. (Just reproduced on 
http://landfill.bugzilla.org/bugzilla-tip/).

Comment 2

[Marc Schumann [:Wurblzap]]

Found another sibling besides bug 240036: bug 218977.

Comment 3

[Christian Reis]

Thanks for opening the bug, Marc. Might I interest you in producing a patch
against CVS HEAD for integration?

Comment 4

[Marc Schumann [:Wurblzap]]

Attachment: Patch to fix crash on non-numerical Hours Worked field

Sure. It's a one-liner, after all.

The patch addresses the Hours Worked issue only. There are several further
occurrences of ThrowUserError without a third parameter. Now who can tell which
ones need to be patched?

Should this bug be marked critical like bug 233124 or a blocker like bug
218977?

Comment 5

[Christian Reis]

Comment on attachment 151166 [details] [diff] [review]
Patch to fix crash on non-numerical Hours Worked field

Yeah.

Comment 6

[Christian Reis]

Upping to major since it's important but in an optional feature.

We should audit the other cases and check if tables are locked when calling
ThrowUseError.

Comment 7

[Vlad Dascalu]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.269; previous revision: 1.268
done

Comment 8

[GavinS]

(In reply to comment #6)
> We should audit the other cases and check if tables are locked when calling
> ThrowUseError.

Has anyone done this audit, or raised a bug to remind us to do it?

Comment 9

[Marc Schumann [:Wurblzap]]

(In reply to comment #8)
> > We should audit the other cases and check if tables are locked when calling
> > ThrowUseError.
> 
> Has anyone done this audit, or raised a bug to remind us to do it?

I just filed bug 276967 for that.