825949 - nsGlobalWindow::InitTimer should just take a uint32_t rather than casting its argument to that

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Bug 824247 hacked around a uint64_t-to-uint32_t conversion warning in  nsGlobalWindow::InitTimer() by adding a static_cast:
https://hg.mozilla.org/integration/mozilla-inbound/diff/0e626106842f/dom/base/nsGlobalWindow.h

However, I don't think we need to have the original variable be a uint64_t in the first place.  It's only called from nsGlobalWindow.cpp, and none of its callers actually pass it a uint64_t.  So, we should just make it take a uint32_t and drop the static_cast.

Comment 1

[Masatoshi Kimura [:emk]]

TimeDuration::ToMilliseconds() returns double value which may be larger than UINT32_MAX, no?

Comment 2

[Daniel Holbert [:dholbert]]

Sure; and that double value could be larger than UINT64_MAX, too, so stuffing it into a 64-bit integer has the same problems.

Regardless, we're already clamping it the 32-bit integer range (with the static_cast); it's just a question of when.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: followup: Convert |realInterval| to be unsigned

This patch converts one of the InitTimer() callers to pass a uint32_t instead of an int32_t.  (It's already known-nonnegative, as enforced by the NS_MAX at the top of this patch's context)

Comment 5

[Daniel Holbert [:dholbert]]

There are a total of 4 callers to this InitTimer() method.
 - 1 of them already passes an uint32_t ("delay")

 - 1 passed an int32_t ("realInterval") but the followup here makes it pass a uint32_t instead.

 - The other two pass a TimeDuration::ToMilliseconds() value, which is a double and which (as noted in comment 1 and comment 2) could be larger than UINT32_MAX or even UINT64_MAX, so we don't really have any type-enforced in-bounds guarantees there.  (So those calls are a little iffy, but this bug's patches don't make them any more or less iffy.)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 697081 [details] [diff] [review]
fix

 
>-  nsresult InitTimer(nsTimerCallbackFunc aFunc, uint64_t delay) {
>+  nsresult InitTimer(nsTimerCallbackFunc aFunc, uint32_t delay) {
while you're here, could you move { to the next line and rename s/delay/aDelay/

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #6)
> while you're here, could you move { to the next line and rename
> s/delay/aDelay/

Done, & pushed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3dab3786d8
and the followup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8b397adc49e9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d3dab3786d8
https://hg.mozilla.org/mozilla-central/rev/8b397adc49e9