497884 - Intermittent failure in test_classifier.html and test_classifier_worker.html

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Townsend [:mossop]]

See http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unittest&logfile=1244812416.1244818784.3640.gz&buildtime=1244812416

These tests will fail with NS_ERROR_NOT_AVAILABLE if they happen to run while the background update of the url-classifier database is in progress. I have a fix in progress but want to try to track bug 486489 a bit more first.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch has two effects.

It allows us to control the url-classifier update interval from a preference and in tests sets that so the earliest an update will happen is after a days running. This should ensure no background updates happen to affect unit tests.

It redirects the url-classifier lookup urls to the local test server. We shouldn't be touching the network during automated tests. The test server will just return errors as nothing exists there, url-classifier should cope with errors ok.

The former is the most sure fix, but we might only want to take the latter on branch since it is a test only change. That should pretty much fix this bug on its own, or at least reduce chances of it happening to pretty much zero.

All of this will also stop bug 486489 from showing up on the tinderbox.

Comment 2

[Daniel Holbert [:dholbert]]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1244946075.1244953165.9759.gz
Linux mozilla-central unit test on 2009/06/13 19:21:15

Comment 3

[Johnathan Nightingale [:johnath]]

(In reply to comment #1)
> Created an attachment (id=383089) [details]
> patch rev 1
> 
> This patch has two effects.
> 
> It allows us to control the url-classifier update interval from a preference
> and in tests sets that so the earliest an update will happen is after a days
> running. This should ensure no background updates happen to affect unit tests.
> 
> It redirects the url-classifier lookup urls to the local test server. We
> shouldn't be touching the network during automated tests. The test server will
> just return errors as nothing exists there, url-classifier should cope with
> errors ok.

Do browser-chrome tests pass with this fix? There are a few tests in there for the about:blocked error pages - the tests themselves are pretty sparse, I'm just curious as to whether the changes you've made cause the MalwareWarden init to be delayed, such that those entries aren't added to the DB (which means the tests would fail, since they would retrieve the actual web content, not the error page).

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Do browser-chrome tests pass with this fix? There are a few tests in there for
> the about:blocked error pages - the tests themselves are pretty sparse, I'm
> just curious as to whether the changes you've made cause the MalwareWarden init
> to be delayed, such that those entries aren't added to the DB (which means the
> tests would fail, since they would retrieve the actual web content, not the
> error page).

Yes all tests pass. The MalwareWarden init continues as normal and so the test data is injected into the database and after that the backend list manager is asked to start requesting table updates. It is just these table updates that are affected.

Comment 5

[David Dahl :ddahl]

Mossop: I think I better fix a few bugs in the safebrowsing module before I review any patches. Is there anyone else that can review this? I should find a couple of basic-functionality bugs in this module work on asap.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 383089 [details] [diff] [review]
patch rev 1

r=sdwilsh

Comment 7

[Dave Townsend [:mossop]]

Fixed on trunk: http://hg.mozilla.org/mozilla-central/rev/f6ba964bf6e3

Comment 8

[(not currently active) Ted Mielczarek]

Should land this on branch, too:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1245433220.1245440926.979.gz

Comment 9

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1245457934.1245462125.8540.gz
OS X 10.5.2 mozilla-1.9.1 unit test on 2009/06/19 17:32:14

Comment 10

[Dave Townsend [:mossop]]

I've landed just the test harness part of this on the 1.9.1 branch, it should be enough on its own to clear up the leaks and this intermittent failure, however I'll still want to land the rest at a later point.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc26906ae71d

Comment 11

[Dave Townsend [:mossop]]

Actual landing: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0da7f782727e

Comment 12

[Legacy TBPL/Treeherder Robot]

dwitte%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283022472.1283022722.11374.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitests-5/5 on 2010/08/28 12:07:52

s: talos-r3-fed64-022
6547 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | failed to block evilWorker.js - got "failure", expected "success"
6548 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | failed to block importScripts('evilWorker.js') - got "failure", expected "success"

Comment 13

[Legacy TBPL/Treeherder Robot]

cdleary%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294458906.1294459600.8755.gz
Rev3 Fedora 12 tryserver opt test mochitests-5/5 on 2011/01/07 19:55:06

s: talos-r3-fed-011
7339 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | Test timed out.

Comment 14

[Daniel Holbert [:dholbert]]

cdleary: Comment 13 is a mis-starring -- #1 it's a timeout and not a failure, and #2 this bug is (purportedly) fixed since 5 months ago.

(I'd suggest filling a new bug, except the starring was for a TryServer build, so it's not necessarily new-bug-worthy as it may have been an issue in the TryServer'd patch)

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #14)

My mistake, didn't mean to star stuff on try, let alone the wrong stuff!