Closed Bug 552386 Opened 11 years ago Closed 11 years ago

test_history_sidebar.js fails between midnight and 2:00 am with new DST settings

Categories

(Toolkit :: Places, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4
Tracking Status
status1.9.2 --- .9-fixed

People

(Reporter: jfkthame, Assigned: mak)

References

Details

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

Attachments

(1 file)

TEST-UNEXPECTED-FAIL | /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | 6 == -1 - See following stack:
JS frame :: /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/head.js :: do_throw :: line 257
JS frame :: /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/head.js :: do_check_eq :: line 287
JS frame :: /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js :: fill_history :: line 158
JS frame :: /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js :: run_test :: line 464
JS frame :: /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/head.js :: _execute_test :: line 151
JS frame :: -e :: <TOP_LEVEL> :: line 1
TEST-INFO | (xpcshell/head.js) | exiting test

We seem to have a sudden bunch of tinderbox failures on this test, and they don't look related to the latest landings. Could this possibly be a delayed result of the change to Daylight Savings time?

All these logs, starting from around midnight PDT, show the same failure:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268636134.1268638185.2510.gz
OS X 10.5.2 mozilla-central debug test xpcshell on 2010/03/14 23:55:34
s: moz2-darwin9-slave13

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268638534.1268640794.8828.gz
 WINNT 5.2 mozilla-central debug test xpcshell on 2010/03/15 00:35:34
s: win32-slave27

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268638266.1268640385.7754.gz
OS X 10.5.2 mozilla-central debug test xpcshell on 2010/03/15 00:31:06
s: moz2-darwin9-slave18

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268637766.1268638946.3988.gz
OS X 10.5.2 mozilla-central opt test xpcshell on 2010/03/15 00:22:46
s: moz2-darwin9-slave10

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268637287.1268639539.5178.gz
Linux mozilla-central debug test xpcshell on 2010/03/15 00:14:47
s: moz2-linux-slave11
OS: Mac OS X → All
hm, it's possible they are DST change related, due to a wrong timediff. but actually looking at the tree it seems random, and that is not compatible with the theory, i guess i will need to see how it evolves.
> +  var DSTCorrection = (dateObj.getTimezoneOffset() - previousDateObj.getTimezoneOffset()) * 60 * 1000;

Isn't this always 0.
Blocks: 485985
(In reply to comment #2)
> > +  var DSTCorrection = (dateObj.getTimezoneOffset() - previousDateObj.getTimezoneOffset()) * 60 * 1000;
> 
> Isn't this always 0.

i added that line because it was not zero and it actually fixed the problem at that time, unless i was completely drunk. But i'm pretty sure i did a lot of testing then. at that time the failure was persistent, not random too.
and actually the failure is here:

157     // Check labels are not repeated.
158     do_check_eq(previousLabels.indexOf(node.title), -1);

November 2009
TEST-PASS | /builds/slave/mozilla-central-macosx-debug-unittest-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | [fill_history : 158] -1 == -1
November 2009

The November container is added twice. This does not look like related to bug 485985 at all.
No longer blocks: 485985
Summary: lots of failures on test_history_sidebar.js → lots of failures on test_history_sidebar.js, month container is added twice to the view
Blocks: 438871
Whiteboard: [orange]
the bug is reproduceable setting to time to 15 March 2010 00:30am. bringing on the time, fixes it (so it should not be an issue as of now on the tree). it is still interesting that the label got duped, so i'll investigate what happens.

PS: do we have a bug about forcing all governments to drop DST? :)
Assignee: nobody → mak77
hm, interesting, November is duped, but February is missing.
Whiteboard: [orange] → [orange:time-bomb]
Summary: lots of failures on test_history_sidebar.js, month container is added twice to the view → test_history_sidebar.js fails between midnight and 2:00 am with new DST settings
Attached patch patch v1.0Splinter Review
So, our code just needs to go back to previous months to get month index and year, this would be also doable with some math (interesting modulus 12 ops), but that will just make the code less readable. I think the easiest solution is to make PR_NormalizeTime just calc date for the first day of the month and don't apply any offset that we should then correct.
Attachment #432641 - Flags: review?(dietrich)
Attachment #432641 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/050887c64183

i'm marking in-testsuite since if this will go crazy, the test will fail again in future. Unfortunatly i can't set the system clock in an automated test, so i've run a bunch of manual testing across this DST change and the 7th November ones.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Comment on attachment 432641 [details] [diff] [review]
patch v1.0

this has minimal risk, and fixes a rare condition that can happen in history sidebar due to DST changes.
Attachment #432641 - Flags: approval1.9.2.3?
Comment on attachment 432641 [details] [diff] [review]
patch v1.0

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.
Attachment #432641 - Flags: approval1.9.2.4?
Comment on attachment 432641 [details] [diff] [review]
patch v1.0

yes I think still makes sense to fix this time bomb failure, to avoid having an orange tree on next dst change. It is a one line change with practically no risks.
Attachment #432641 - Flags: approval1.9.2.7?
Comment on attachment 432641 [details] [diff] [review]
patch v1.0

a=LegNeato for 1.9.2.8. Please land this on mozilla-1.9.2 default.
Attachment #432641 - Flags: approval1.9.2.8+
Attachment #432641 - Flags: approval1.9.2.7?
Attachment #432641 - Flags: approval1.9.2.7-
Whiteboard: [orange:time-bomb] → [orange][time-bomb]
Whiteboard: [orange][time-bomb] → [time-bomb]
You need to log in before you can comment on or make changes to this bug.