Closed
Bug 825949
Opened 12 years ago
Closed 12 years ago
nsGlobalWindow::InitTimer should just take a uint32_t rather than casting its argument to that
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files)
1.75 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
TimeDuration::ToMilliseconds() returns double value which may be larger than UINT32_MAX, no?
Assignee | ||
Comment 2•12 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•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
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•12 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•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 6•12 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•12 years ago
|
Attachment #697085 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 7•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d3dab3786d8 https://hg.mozilla.org/mozilla-central/rev/8b397adc49e9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•