Closed
Bug 1469104
Opened 6 years ago
Closed 6 years ago
testcase in browser_sanitize-timespans.js will fail if it runs within a second after midnight
Categories
(Toolkit :: Places, enhancement, P1)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
References
Details
(Whiteboard: [fxsearch])
Attachments
(1 file, 1 obsolete file)
I recently noticed that browser_sanitize-timespans.js is guaranteed to fail if it runs at exactly the wrong time of day; this will appear as a rare intermittent orange. Specifically, it will fail if the code here https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/browser/base/content/test/sanitize/browser_sanitize-timespans.js#453-457 executes within the first second after midnight. In this case, the use of setSeconds(1) means that we are actually giving the visit a time of 00:00:01.xx, while the actual current time is 00:00:00.xx. (Note that the milliseconds are left untouched here.) So the visit is being given a time that is 1 second in the future, and this causes the validatePageInfo function in PlacesUtils to throw a TypeError at https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/toolkit/components/places/PlacesUtils.jsm#1108 because history is supposed to be in the past. Here's a try job that happened to hit the "unlucky second", demonstrating that it really can happen: https://treeherder.mozilla.org/#/jobs?repo=try&revision=494edb82f2ba404fed034bf865a15d47729be1b7&selectedJob=183431587 I believe we can fix this by using setSeconds(0) instead of setSeconds(1) here. If we're concerned about the edge case where the visit is at *exactly* midnight, we could use setMilliseconds(1) to ensure that it remains still definitely post-midnight; this will not cause a problem because of the TIMERS_RESOLUTION_SKEW_MS (16ms) tolerance allowed by the check in validatePageInfo.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8985746 -
Flags: review?(mak77)
Assignee | ||
Comment 2•6 years ago
|
||
Actually, we should fix all three instances of this pattern, not just the one I happened to stumble across. (Note that the fix for bug 1469518 is required before this can land.)
Attachment #8986132 -
Flags: review?(mak77)
Assignee | ||
Updated•6 years ago
|
Attachment #8985746 -
Attachment is obsolete: true
Attachment #8985746 -
Flags: review?(mak77)
Comment 3•6 years ago
|
||
Comment on attachment 8986132 [details] [diff] [review] Fix testcase in browser_sanitize-timespans to not fail immediately after midnight Review of attachment 8986132 [details] [diff] [review]: ----------------------------------------------------------------- thank you!
Attachment #8986132 -
Flags: review?(mak77) → review+
Updated•6 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxsearch]
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef348f3f604c Fix testcase in browser_sanitize-timespans to not fail immediately after midnight. r=mak
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ef348f3f604c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•