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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Jonathan Kew [:jfkthame]]

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

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[John P Baker]

> +  var DSTCorrection = (dateObj.getTimezoneOffset() - previousDateObj.getTimezoneOffset()) * 60 * 1000;

Isn't this always 0.

Comment 3

[Marco Bonardo [:mak]]

(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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

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? :)

Comment 6

[Marco Bonardo [:mak]]

hm, interesting, November is duped, but February is missing.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Legacy TBPL/Treeherder Robot]

dougt
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1277968418.1277969466.7520.gz
Rev3 Fedora 12 mozilla-central opt test xpcshell on 2010/07/01 00:13:38

s: talos-r3-fed-024
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora-opt-u-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora-opt-u-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | 8 == 9 - See following stack:

Comment 13

[Legacy TBPL/Treeherder Robot]

dougt
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1277968994.1277970102.10338.gz
Rev3 Fedora 12x64 mozilla-central opt test xpcshell on 2010/07/01 00:23:14

s: talos-r3-fed64-038
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora64-opt-u-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora64-opt-u-xpcshell/build/xpcshell/tests/test_places/unit/test_history_sidebar.js | 8 == 9 - See following stack:

Comment 14

[LegNeato]

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.

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7612534ea1ef