Closed Bug 825949 Opened 7 years ago Closed 7 years ago

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


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

Not set





(Reporter: dholbert, Assigned: dholbert)




(2 files)

Bug 824247 hacked around a uint64_t-to-uint32_t conversion warning in  nsGlobalWindow::InitTimer() by adding a static_cast:

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.
TimeDuration::ToMilliseconds() returns double value which may be larger than UINT32_MAX, no?
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.
Attached patch fixSplinter Review
Assignee: nobody → dholbert
Attachment #697081 - Flags: review?(bugs)
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)
Attachment #697085 - Flags: review?(bugs)
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.)
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 697081 [details] [diff] [review]

>-  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/
Attachment #697081 - Flags: review?(bugs) → review+
Attachment #697085 - Flags: review?(bugs) → review+
(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:
and the followup:
Flags: in-testsuite-
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.