111552 - nsDialogParamBlock.cpp

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Kai Engert [:KaiE:]]

I think class nsDialogParamBlock has a dangerous behaviour. It does allow itself
to be filled with a certain amount of values, but does not grow automatically.

While it gives an error code for out of bounds operations, and most code in
Mozilla seems to check for the error code, I hardly see any code that uses
SetNumberStrings to prepare the required size.

This behaviour led to bug 110420 - of course, I should have read the
implementation of nsDialogParamBlock.cpp first, but I didn't. I programmed by
example, reading other code that also did not call SetNumberStrings.

The dangerous part is that during development you will often not see any
problems, because you have less data than in a real world szenario.

I guess the interface nsIDialogParamBlock is fixed and must not be extended, so
we can't add a method SetNumberInts.

What do you think, should we assume that programmers should read the
implementation of this class, or should we try to make it failsafe, by
automatically growing, and giving assertions in debug mode?

I suggest to give assertions when the class allocates the default number of
strings, enforcing developers to think about the number of strings they want,
therefore encouraging them to call SetNumberStrings.

We can't do that for the integers if my assumption is right and the interface is
fixed. I suggest to extend the class with an automatic growth mechanism for
integers.

Comment 1

[Mike Shaver (:shaver emeritus)]

nsDialogParamBlock isn't frozen, so we should be happy to take changes that
improve it.  SetNumberInts sounds like such an improvement, though I must admit
that on first glance this whole param-block deal seems a bit overwrought. 
nsISupportsArray and the wrapper interfaces?  I digress...

Don't use an assertion for the unspecified-string-count case if you don't really
mean it to be an error.  I'd be all for having ParamBlock fail miserably (all
params are "PILOT_ERROR", perhaps) when misused and returning an error code, but
assertions should be for "can't happen!" cases.

Growing dynamically for both strings and integers sounds pretty
programmer-friendly, and it would insulate interface consumers from the
implementation a bit better.

(And fix the method casing!)

Comment 2

[Stephane Saux]

Mike,
There are other problems with the SetNumberStrings.
SetNumberString(i) will cause of array of size i to be created.
So if the semantics is that you want to store i params,
and that people seem to use SetString(1, string) to store the first params,
when one calls SetString(i, string) the index is out of bound.

The issues here are to specify fully the API:
- Are params indexed started with 1 or 0.  What have people been doing?
I suspect that the common usage is 1, but using 0 would work today.
- The safest thing to do is change the behavior of SetNumberString(i) to
allocate an i+1 sized array. This would waste four bytes.  However people who
use SetNumString for paramblock of 1 or 2 already save (16 - (1 or 2)) * 4 bytes
since the default is to create a 16 sized array. 
- The "right" thing if we ascertain that indeed 1 is the accepted usage, change
InBound() to check that i > 0 rather than just i>=0. and change the
implementation of SetString to assign to the i-1 position in the array, and
similarly GetParam. This would not waste any storage, and would not require any
changes to the rest of the code.  It's more risky in that we may not realize
that some folks indeed use zero. InBound would fail for these people.

We have bugs that are related to this inconsistent implementation. See bug 115304

Comment 3

[Stephane Saux]

I'd be happy with better documentation:
Indexes start at zero.

If SetNumberStrings is not called, you get 16 strings.  This is probably good
enough for param block that have a small, fixed number of strings.
If one will have a large number of parameters one should figure out how many and
explicitely call SetNumberString.

It may be good usage to call SetNumberString when one has a small fixed number
of string params. This will limit the size of the string array.

There is a fixed number of integer params (8).  this cannot be changed.