16610 - 3 methods don't throw INDEX_SIZE_ERR

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DESCRIPTION:  The methods insertData(), deleteData(), and replaceData() of the
Text interface don't throw INDEX_SIZE_ERR at all.  (Note that they should throw
it only when outside of the range 0 <= n <= length, NOT 0 <= n < length!!  I've
been through this before on the things that do throw INDEX_SIZE_ERR...)

STEPS TO REPRODUCE:
 * Load http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/exceptions
   or http://www.fas.harvard.edu/~dbaron/dom/test/one-core-html/exceptions
 * Hit "test exceptions"

ACTUAL RESULTS:
 * All but the first 8 lines of the testing of the Text interface (which starts
a little more than halfway through) are red.

EXPECTED RESULTS:
 * They should be green.

DOES NOT WORK CORRECTLY ON:
 * Linux, viewer, 1999-10-15-11-M11

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem I mentioned in the description (that you should avoid when fixing
this) was bug 9777.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to fix bug

Comment 3

[vidur (gone)]

Reassigning to Waqar, since he's been in that code quite a lot recently.

Comment 4

[gerardok]

Chris, please could you verify this one. Thanks.

Comment 5

[ckritzer (gone)]

This bug is still open as of 1999112809 Linux build.
Reopening bug.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  It looks fine to me in Linux mozilla 1999-11-28-08-M12.

Comment 7

[waqar]

This bug has been fixed. I am marking it as.

Comment 8

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-03-07-09 Commercial build
- MacOS9 2000-03-07-08 Commercial build
- Win98 2000-03-07-09 Commercial build

Comment 9

[Bob Clary [:bc] (inactive)]

replaceData(offset,count,arg) does not throw exception when count is negative.

Visit http://www.mindspring.com/~bobclary/domtest2/tc.html using a recent build
of Mozilla, select the following test cases: tc_ccda501, tc_ccom501, tc_ctxt501
and submit.

To view a report click on a link in the left frame.  Due to a scrollbar bug you
will need to click into the right frame and use your keyboard to navigate.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Reopening based on above comments.  Perhaps it's because it's a PRUint32 rather
than PRInt32.

Comment 11

[Bob Clary [:bc] (inactive)]

dbaron is right.

153 void CharacterData::replaceData(Int32 aOffset, Int32 aCount,
154 const String& aSource)
155 {
156 nsCharacterData->ReplaceData(aOffset, aCount, aSource.getConstNSString());
157 }
158

But

407 nsresult
408 nsGenericDOMDataNode::ReplaceData(nsIContent *aOuterContent, PRUint32 aOffset,
409 PRUint32 aCount, const nsAReadableString& aData)
410 {
411 nsresult result = NS_OK;
412 413 // sanitize arguments 414 PRUint32 textLength = mText.GetLength();
415 if ((aOffset > textLength) || (aOffset < 0) || (aCount < 0)) {
416 return NS_ERROR_DOM_INDEX_SIZE_ERR;
417 }
418

Comment 12

[waqar]

Reassigning to jst.

Comment 13

[Johnny Stenback (:jst)]

The DOM spec defines the integer input arguments to replaceData() as "unigned
long" so passing in -1 is invalid and should be caught by the language compiler
(it you're using a language that is strongly typed) but in the case of JS we
fail to catch this problem. We simply do a JS_ValueToInt32() on the arguments
and we pass a PRUint32 typecasted to int32 so we loose the fact that the
argument is negative, in stead we get the argument as 0xffffffff and that's a
valid count as far as replaceData() goes...

If we wanto get this right we need to convert the JSVal to a signed quantity
first to check if it's negative or not and if it is then we need to throw an
exception (note, IMO the DOM exception INDEX_SIZE_ERR is not the correct
exception here since we never even get into DOM code before we realize that the
input is invalid, a JS specific exeption saying that the value is out of range
would be the correct exception to throw). If the value is not negative we need
to get it as a unsigned quantity and continue calling replaceData(). I don't
really wanna fix this now since the glue code where this fix would go will be
removed soon anyway so I'll leave this open for now, XPConnect should deal with
things like this in the future. Setting milestone to mozilla1.1 for now.

Comment 14

[ckritzer (gone)]

->gerardok

Comment 15

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 16

[carnold]

An approach would be to create a signed wrapper for
nsGenericDOMNode::ReplaceData to complement the unsigned implementation:

Since you already have this:


 nsresult
408 nsGenericDOMDataNode::ReplaceData(nsIContent *aOuterContent, PRUint32 aOffset,
409 PRUint32 aCount, const nsAReadableString& aData)
410 {
411 nsresult result = NS_OK;
412 413 // sanitize arguments 414 PRUint32 textLength = mText.GetLength();
415 if ((aOffset > textLength) || (aOffset < 0) || (aCount < 0)) {
416 return NS_ERROR_DOM_INDEX_SIZE_ERR;
417 }
....

You could add:

nsresult
nsGenericDOMDataNode::ReplaceData(nsIContent *aOuterContent, PRint32 aOffset,
PRint32 aCount, const nsAReadableString& aData)
{
nsresult result = NS_OK;
if((aOffset < 0) || (aCount < 0)) {
   return NS_ERROR_DOM_INDEX_SIZE_ERR;
}
return ReplaceData(aOuterContent, (PRUint32) aOffset, (PRUint32) aCount, aData);
}


You could remove the spurious < 0's in the unsigned implementations.  If code
has (hopefully safely) cast down to unsigned numbers, then they will bypass the
signed wrapper. 

Comment 17

[Johnny Stenback (:jst)]

Method over loading requires API changes to frozen API's, and it's also
impossible to overload methods in XPIDL.

Comment 18

[Sivakiran Tummala]

Does characterdata.substringdata(10, -9) fall in this catogery. 

Comment 19

[Johnny Stenback (:jst)]

Yes.

Comment 20

[Fabian Guisset]

bug 43164 also covers 
characterdata.substringdata(10, -9)

Comment 21

[Sivakiran Tummala]

The INDEX_SIZE_ERR expection is also not thrown for these two cases.
HTMLTableSectionElement.insertRow and HTMLTableSectionElement.deleteRow

Comment 22

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 23

[Peter Van der Beken [:peterv]]

*** Bug 43164 has been marked as a duplicate of this bug. ***

Comment 24

[Peter Van der Beken [:peterv]]

Taking.

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 26

[Peter Van der Beken [:peterv]]

Comment on attachment 131798 [details] [diff] [review]
v1

Let's see what jst thinks about this approach.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 131798 [details] [diff] [review]
v1

Hmm, I'd much rather see this done in XPConnect, where it IMO belongs. Putting
this anywhere else opens the door for errors in situations where JS passes a
negative value to a C++ function that then ends up calling this function.

So sr-, unless someone convinces me that this really doesn't belong in
XPConnect.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 29

[Peter Van der Beken [:peterv]]

Attachment 131811 [details] [diff] fixes this in XPConnect, though I'm open to suggestions (I'm
no expert on how to do conversions in the JS engine). I would personally prefer
not putting this in XPConnect, if only because it would throw the wrong exception.
CC'ing some XPConnect and JS people. Do we want to enforce the signedness of
integer arguments when going from JS to native?

Comment 30

[David Bradley]

1. If we're going to do this much, shouldn't we go ahead and test both sides of
the valid range for all the types?
2. I wonder how much code currently works because of this "hole" and won't after
it's plugged.

Comment 31

[Brendan Eich [:brendan]]

I don't think we should be changing this post-mozilla-1.0.  The risk of breaking
backwards compatibility is too great.

What other methods might want this change?  Why shouldn't this change be done in
the DOM layer?

/be

Comment 32

[Peter Van der Beken [:peterv]]

Comment 21 lists two other DOM methods that might need this. I don't think we
want to impose this on methods outside the DOM. I think the DOM case is a bit
weird, in that the recommendation's idl specifies these arguments as unsigned
long, but then also specifies what needs to happen if negative arguments get
passed in. For other cases, I personally would expect the current
conversion-to-positive behaviour to happen.
I could add a better check that this isn't called through another C++ function
on the same object by checking the interface this gets called through and with
the methodindex (I think). Not sure we'd want that in non-debug builds though.

Comment 33

[Peter Van der Beken [:peterv]]

Jst, do you still oppose attachment 131798 [details] [diff] [review]?

Comment 34

[Johnny Stenback (:jst)]

I think I'd prefer that we either do this in the binding layer, or we don't do
it at all. I agree that it's way late to make a change like this in the binding
layer, unless we can limit such a change to DOM objects, which we could, but I
don't think that'll be very clean or low-impact, or worth the trouble, even.

Comment 35

[Peter Van der Beken [:peterv]]

Ok, then I guess this is a WONTFIX. Sad.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should reopen this. Chaning this on the DOM side has effectively the same effect as the v2 patch since it'll only affect XPIDL interfaces, of which there are very few that content interacts with other than the DOM.

Comment 37

[Jan]

Bug is still actual in FF3.