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

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla20
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
TimeDuration::ToMilliseconds() returns double value which may be larger than UINT32_MAX, no?
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 697081 [details] [diff] [review]
fix
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #697081 - Flags: review?(bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 697085 [details] [diff] [review]
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)
Attachment #697085 - Flags: review?(bugs)
(Assignee)

Comment 5

5 years ago
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.)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All

Comment 6

5 years ago
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/
Attachment #697081 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #697085 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

5 years ago
(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
Flags: in-testsuite-

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/4d3dab3786d8
https://hg.mozilla.org/mozilla-central/rev/8b397adc49e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.