371611 - crash nsIScriptableDateFormat::FormatDate for undefined year, month, day

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: testcase

crash nsIScriptableDateFormat::FormatDate for undefined year, month, day.

Example:

const nsIScriptableDateFormat =
            Components.interfaces.nsIScriptableDateFormat

    var dateConv = Components.classes["@mozilla.org/intl/scriptabledateformat;1"].
                   getService(nsIScriptableDateFormat);
    dateConv.FormatDate("", nsIScriptableDateFormat.dateFormatLong,
                        undefined, undefined, undefined);

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

Am I right in assuming the problem here is we're passing a 0 (undefined becomes 0 after conversion) for the year, month and day which is invalid. Wouldn't the fix therefore be something like this? In the case where either the year, month or day is 0 should we return |NS_ERROR_ILLEGAL_VALUE|?

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

+    return NS_ERROR_ILLEGAL_VALUE; // invalid arg value

fwiw we have NS_ERROR_INVALID_ARG which'd the comment redundant :) (and a unit test would be nice)

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #2)
> +    return NS_ERROR_ILLEGAL_VALUE; // invalid arg value
> 
> fwiw we have NS_ERROR_INVALID_ARG which'd the comment redundant :) (and a unit
> test would be nice)
> 

Fair enough - I was following the standard per line 148 so should that be changed also?

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

* As suggested by Christian - use |NS_ERROR_INVALID_ARG| over |NS_ERROR_ILLEGAL_VALUE| as its self-explanatory (also changed the second instance of this to maintain a standard).

Comment 5

[Robert Sayre]

add an xpcshell test for this, please.

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #5)
> add an xpcshell test for this, please.
> 

Could we possibly add the xpcshell test to the pre-existing LocaleSelfTest.cpp file?

Comment 7

[Robert Sayre]

(In reply to comment #6)
> (In reply to comment #5)
> > add an xpcshell test for this, please.
> > 
> 
> Could we possibly add the xpcshell test to the pre-existing LocaleSelfTest.cpp
> file?
> 

if you really wanted to. however, getting JS coverage would be better. you know where to find me on IRC if you need help

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #7)
> if you really wanted to. however, getting JS coverage would be better. you know
> where to find me on IRC if you need help
> 

Will do. I'll take a look at writing a test for this tomorrow when I get home from school in that case. Thanks!

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Test v1

Test v1

Comment 10

[Robert Sayre]

Comment on attachment 256479 [details] [diff] [review]
Test v1

looks good to me. but have the module owners review your tests. Ideally, they should be in the patch with the code.

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #10)
> (From update of attachment 256479 [details] [diff] [review])
> looks good to me. but have the module owners review your tests. Ideally, they
> should be in the patch with the code.
> 

Thanks for the comment. I'll remember to add them with the main patch from now on.

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 256387 [details] [diff] [review]
Patch v1.1

r=me, but I'm not a superreviewer

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 256479 [details] [diff] [review]
Test v1

While you're here, why not add tests for too-large values of month and day? r=me either way.

Comment 14

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Test v2

Test v2

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 256595 [details] [diff] [review]
Test v2

+  /* Testing if we throw instead of crashing when we are passed 0s. */

either change the comment to say "undefined" or the code to pass zero. I'd prefer the latter.
+  catch (ex if (ex.result == NS_ERROR_INVALID_ARG)) {

where is that constant defined?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

and you need to change the makefile too so that this unit test runs, don't you?

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Simon, do you have any idea why |make check| doesn't work in the mozilla/intl/tests/ folder?

I've got an updated patch and am unable to test it because I can't seem to get make check to run without error.

Comment 18

[Robert Sayre]

(In reply to comment #17)
> Simon, do you have any idea why |make check| doesn't work in the
> mozilla/intl/tests/ folder?

also, |make| doesn't work in there either

Comment 19

[Simon Montagu :smontagu]

Grrr. It used to work, please file a bug against me. (You mean mozilla/intl/locale/tests, right?)

Comment 20

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #19)
> Grrr. It used to work, please file a bug against me. (You mean
> mozilla/intl/locale/tests, right?)
> 

Yes, that's it.

Comment 21

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Complete patch (v1)

Complete patch (v1)

* Includes the changes too the makefile, the test and the patch (as well as the makefile changes specified in bug 371990).

Comment 22

[Robert Sayre]

Attachment: Makefile tweaked

with this patch, the test runs as expected in |make check|.

However, the test fails:

*** CHECK FAILED: FormatDate didn't reject invalid month value.

Comment 23

[Robert Sayre]

Comment on attachment 256728 [details] [diff] [review]
Makefile tweaked

er, that patch skips the new unit/ directory in Ryan's patch but it's otherwise right.

Comment 24

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Complete patch (v2)

Complete patch (v2)

* With Robert's changes (thanks for all your help).

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 256731 [details] [diff] [review]
Complete patch (v2)

did you actually test this? the makefile still needs the includes.

Comment 26

[Simon Montagu :smontagu]

Comment on attachment 256731 [details] [diff] [review]
Complete patch (v2)

My fault for ambiguous phrasing in bug 371990 comment 5: when I said "I think the lines between foo and bar can be removed" I should have added "not including the lines quoted"

Comment 27

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Complete patch (v2.2)

Complete patch (v2.2)

* Fixed the makefile error but the test still fails with |*** CHECK FAILED: FormatDate didn't reject invalid month value.|

I don't know why this occurs, Simon - any ideas and is it still OK to request review on this patch?

Comment 28

[Simon Montagu :smontagu]

(In reply to comment #27)
> * Fixed the makefile error but the test still fails with |*** CHECK FAILED:
> FormatDate didn't reject invalid month value.|
> 
> I don't know why this occurs, Simon - any ideas

This turns out to be a feature of mktime:

"In addition to computing the calendar time, mktime normalizes the supplied tm structure. The original values of the tm_wday and tm_yday components of the structure are ignored, and the original values of the other components are not restricted to the ranges indicated in the definition of the structure."

So too-large values of month and day are not expected to fail and there's no point in testing for them. Sorry for the bad suggestion.

Comment 29

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Complete patch (v2.3)

(In reply to comment #28)
> So too-large values of month and day are not expected to fail and there's no
> point in testing for them. Sorry for the bad suggestion.
> 

Ah OK, in that case I've removed the unneeded code from the test.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 256807 [details] [diff] [review]
Complete patch (v2.3)

+  const nsIScriptableDateFormat = Ci.nsIScriptableDateFormat;

well ok, I wouldn't bother with this, it's not much shorter than Ci.nsIScriptableDateFormat

Comment 31

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.4

patch v2.4

* Fixed the nits and hopefully ready for checkin.

Comment 32

[Ryan Jones-Ward [:sciguyryan]]

This was checked in by Sayre so thanks!