825247 - nsHTMLInputElement::ConvertNumberToString uses JS::Value::toNumber unsafely

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Test

STR:

1. Enable the dom.experimental_forms pref.
2. Load attached test case.

Result:

Assertion failure: isNumber(), at ../../../../dist/include/jsapi.h:474

Offending code was added here: <https://hg.mozilla.org/mozilla-central/rev/78970aaa8008#l1.150>

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that the above problem was pointed out in the review comments on bug 769370.

There are a few other problems here too, actually:

1)  Per spec, the setter should throw if passed anything other than a Date object or null.  But the implementation here just sets the value to empty string if passed a non-date.

2)  A false return value from the JS::Call is silently swallowed without clearing the pending exception, which will cause the caller to randomly end up with an exception at some later random point.  Is the desired behavior to catch the exception or rethrow it?

3)  The JS::Call stuff in nsHTMLInputElement::SetValue has similar problems, and additionally fails to do the right thing if the page messes with Date.prototype.

4)  And similar problems in nsHTMLInputElement::ConvertStringToNumber.  Nte that JS_NewDateObjectMsec will happily return null on OOM.

We really need some non-JS date representation (which could even sit on top of the JS engine's date stuff, if needed, but wouldn't involve manual JSAPI)....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and I would love feedback on how to represent Date in WebIDL bindings to make all this less painful!

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Note that the above problem was pointed out in the review comments on bug
> 769370.
> 
> There are a few other problems here too, actually:
> 
> 1)  Per spec, the setter should throw if passed anything other than a Date
> object or null.  But the implementation here just sets the value to empty
> string if passed a non-date.

bug 826305

> 2)  A false return value from the JS::Call is silently swallowed without
> clearing the pending exception, which will cause the caller to randomly end
> up with an exception at some later random point.  Is the desired behavior to
> catch the exception or rethrow it?

I think catching it should be the correct behaviour. I mean, ideally, we should use dedicated internal methods and not depend on the prototype.

> 3)  The JS::Call stuff in nsHTMLInputElement::SetValue has similar problems,
> and additionally fails to do the right thing if the page messes with
> Date.prototype.
> 
> 4)  And similar problems in nsHTMLInputElement::ConvertStringToNumber.  Nte
> that JS_NewDateObjectMsec will happily return null on OOM.

I am going to attach a patch that fixes those issues.

> We really need some non-JS date representation (which could even sit on top
> of the JS engine's date stuff, if needed, but wouldn't involve manual
> JSAPI)....

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch

This is fixing the exceptions and the proto change issues.

I removed NaN checks because it seems that this is working as expected (see tests). Let me know if I missed something.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 697487 [details] [diff] [review]
Patch

>         JSObject* date = JS_NewDateObjectMsec(ctx, 0);
>+        NS_ENSURE_TRUE(date, false);

That leaves a pending unreported exception on ctx, no?  You probably want a JS_ClearPendingException(ctx) here too.

> I removed NaN checks because it seems that this is working as expected (see
> tests)

It's not clear to me that the test is testing the relevant behavior.

Specifically, what happens with this patch in a web page if you don't do any messing with the proto but do:

  element.valueAsDate = new Date(NaN);

What does element.valueAsDate return after that?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> > I removed NaN checks because it seems that this is working as expected (see
> > tests)
> 
> It's not clear to me that the test is testing the relevant behavior.
> 
> Specifically, what happens with this patch in a web page if you don't do any
> messing with the proto but do:
> 
>   element.valueAsDate = new Date(NaN);
> 
> What does element.valueAsDate return after that?

According to the specs:
  element.valueAsDate = new Date(NaN);
should set the value to the empty string.

Which means, that:
  element.valueAsDate;
would return null.

We have a test checking that:
  element.valueAsDate = new Date(NaN);
  element.value === "";
And another doing:
  element.value = "";
  element.valueAsDate === null;

So I think this behaviour is tested.

Basically, the question I had can be summarized by: would foo.isNumber() would return true even if MOZ_DOUBLE_IS_NaN(foo) returns true?

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 8

[Boris Zbarsky [:bzbarsky]]

>According to the specs:
>  element.valueAsDate = new Date(NaN);
>should set the value to the empty string.

Yes, agreed.

> We have a test checking that:

Sounds good.

> would foo.isNumber() would return true even if MOZ_DOUBLE_IS_NaN(foo) returns true?

Yes, it would.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698619 [details] [diff] [review]
Patch v3

r=me

Comment 11

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a853903e4470

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a853903e4470

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mounir, don't forget to land this on Aurora!

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 698619 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 769370
User impact if declined: unexpected behaviour
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Mounir, don't forget to land this on Aurora!

Thank you for the heads up :)

Comment 16

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f74542c3678