Closed Bug 825247 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: Ms2ger, Assigned: mounir)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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>
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)....
Oh, and I would love feedback on how to represent Date in WebIDL bindings to make all this less painful!
Depends on: 826302
No longer depends on: 826302
(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)....
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #697487 - Flags: review?(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?
(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?
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #697487 - Attachment is obsolete: true
Attachment #697487 - Flags: review?(bzbarsky)
Attachment #698456 - Flags: review?(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.
Attached patch Patch v3Splinter Review
Attachment #698456 - Attachment is obsolete: true
Attachment #698456 - Flags: review?(bzbarsky)
Attachment #698619 - Flags: review?(bzbarsky)
Comment on attachment 698619 [details] [diff] [review]
Patch v3

r=me
Attachment #698619 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a853903e4470
Flags: in-testsuite+
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/a853903e4470
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla20 → mozilla21
Mounir, don't forget to land this on Aurora!
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
Attachment #698619 - Flags: approval-mozilla-aurora?
(In reply to Boris Zbarsky (:bz) from comment #13)
> Mounir, don't forget to land this on Aurora!

Thank you for the heads up :)
Attachment #698619 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.