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)

enhancement

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.
Depends on: 1469518
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)
Attachment #8985746 - Attachment is obsolete: true
Attachment #8985746 - Flags: review?(mak77)
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+
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
https://hg.mozilla.org/mozilla-central/rev/ef348f3f604c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.