Closed Bug 467997 Opened 16 years ago Closed 13 years ago

browser_sanitize-timespans.js random failure when crossing midnight

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0

People

(Reporter: robert.strong.bugs, Assigned: mak)

References

Details

(Keywords: intermittent-failure, Whiteboard: [orange:time-bomb])

Attachments

(2 files, 1 obsolete file)

patch v1.1
2.39 KB, patch
Details | Diff | Splinter Review
2nd attempt
974 bytes, patch
sdwilsh
: review+
Details | Diff | Splinter Review 
From
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228430608.1228434980.30800.gz&fulltext=1

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | Pretend visit to today.com should now be deleted
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | 'Today' download should now be deleted

There were no checkins prior to this build
I don't think we've seen this again, so I'm going to resolve it as INCOMPLETE.  Probably just random.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → INCOMPLETE
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | Pretend visit to today.com should now be deleted
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | 'Today' download should now be deleted

WINNT 5.2 mozilla-central test everythingelse
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258616566.1258620109.16348.gz
Blocks: 438871
Status: RESOLVED → REOPENED
OS: Mac System 9.x → All
Hardware: PowerPC → All
Resolution: INCOMPLETE → ---
Summary: browser_sanitize-timespans.js test FAILED → browser_sanitize-timespans.js random failure
Whiteboard: [orange]
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1259048570.1259050907.2072.gz
OS X 10.5.2 mozilla-central opt test everythingelse on 2009/11/23 23:42:50  
s: moz2-darwin9-slave16
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | Pretend visit to today.com should now be deleted
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | 'Today' download should now be deleted


The very first log isn't around anymore, but the other two failures were runs that were running right around midnight.  I think the problem may be that this test fails when it's run right at midnight or something like that.
Summary: browser_sanitize-timespans.js random failure → browser_sanitize-timespans.js random failure (failure when crossing midnight?)
Whiteboard: [orange] → [orange][orange:time-bomb]
WINNT 5.2 mozilla-central opt test mochitest-other on 2010/03/14 23:51:35  
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268635895.1268636790.31473.gz
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | Pretend visit to today.com should now be deleted
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | 'Today' download should now be deleted
yes, we could add history and downloads just before midnight, and when sanitize will run after midnight they won't be removed because they are not "today". A simple workaround could just be to skip the test if it runs like after 23:55.
Attached patch patch v1.0 (obsolete) — — Splinter Review 
this is a different path, i just check if today is still today before running the test.
Assignee: nobody → mak77
Attachment #439676 - Flags: review?
Attachment #439676 - Flags: review? → review?(adw)
Attachment #439676 - Flags: review?(adw) → review+
Comment on attachment 439676 [details] [diff] [review]
patch v1.0

Cache the isToday() check right after the call to s.sanitize(), move the three conditional ok()s into one if (today) block, and expand on the comment a little, e.g., "Careful: Skip these checks if..."

r=adw
Attached patch patch v1.1 — — Splinter Review 
Address comments
Attachment #439676 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/74dfe85acfe0
Status: REOPENED → RESOLVED
Closed: 16 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Summary: browser_sanitize-timespans.js random failure (failure when crossing midnight?) → browser_sanitize-timespans.js random failure when crossing midnight
It's not directly germane to this fix, but I notice that this test contains the line:

var minutesSinceMidnight = new Date().getMinutes();

with minutesSinceMidnight used as a guard around various tests. (c.f. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_sanitize-timespans.js#14 and then http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_sanitize-timespans.js#182 )

new Date().getMinutes() returns minutes since the top of the hour, not since midnight, so all the if blocks testing for minutes > 60 will permanently fail, which means we're not running those tests.
(In reply to comment #10)
> new Date().getMinutes() returns minutes since the top of the hour, not since
> midnight, so all the if blocks testing for minutes > 60 will permanently fail,
> which means we're not running those tests.

pretty cool, i'd say to file a new bug please, since it's not really related to this orange.  Not even an orange bug actually, but a real test bug.
(In reply to comment #11)
> file a new bug please

I filed bug 561528.
Flags: in-testsuite+
Reopening this, as comment #13 looks just like the original failures here - apparently it hasn't been sufficiently fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
well, that does not look like being a midnight crossing at all.
The test run crossed midnight: looks like the robot has put my local time into comment #13, but in MV it started at 23:46, finished 00:18.
hu, weird, thanks for the clarification
Assignee: mak77 → nobody
Attached patch 2nd attempt — — Splinter Review 
Ah, I must be blind, I think I got the problem (and it took only 2 years!). The point I thought about is that downloads and history are not cleared, but form history is, then there must be a difference.

When we add history and download entries we do:

let today = new Date();
today.setHours(0);
today.setMinutes(0);
today.setSeconds(30);

While form history does:

today.setSeconds(1);

sanitize() will clear from 00:00:00 to Date.now()

Now, think that sanitize() runs between 00:00:00 and 00:00:30... It actually cannot remove history and downloads because they are in the future (damn time machines).
Form history could fail if sanitize would run exactly at 00:00:00, but then our isToday script would skip the test thinking it is across midnight and not worth it.

there are various possible solutions to the problem, but I'll go for the smallest patch, just changing (30) to (1).
Assignee: nobody → mak77
Status: REOPENED → ASSIGNED
Attachment #515068 - Flags: review?(sdwilsh)
Comment on attachment 515068 [details] [diff] [review]
2nd attempt

r=sdwilsh
Attachment #515068 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/61d11ecb169b
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a5 → mozilla2.0
You'll wanna fix up this comment too, or just remove it, the code _is_ right above it:

     startTime: today.valueOf() * 1000,  // 12:00:30am this morning, in uSec
makes sense, will followup a comment fix, thanks.
Whiteboard: [orange][orange:time-bomb] → [orange:time-bomb]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: