739687 - [SeaMonkey] test_classifier.html and test_classifier_worker.html fail

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1332863004.1332865068.15862.gz
WINNT 5.2 comm-central-trunk debug test mochitests-5/5 on 2012/03/27 08:43:24
{
9670 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier.html | Should not load bad javascript - got loaded malware javascript!, expected untouched
9671 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier.html | Should not load bad css - didn't expect hidden, but got it

9674 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | failed to block evilWorker.js - got failure, expected success
9675 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
}

Bug 738883 packages url-classifier files.
Bug 738884 enables MOZ_URL_CLASSIFIER=1.

Now the related test are enabled :-) but fail :-(
Would they miss to set browser.safebrowsing.malware.enabled preference?

Comment 1

[Philip Chee]

http://mxr.mozilla.org/comm-central/search?string=safebrowsing.malware.enabled

> Would they miss to set browser.safebrowsing.malware.enabled preference?
Possibly.

The unit tests but not the mochitests do the following:
// Enable malware/phishing checking for tests
prefBranch.setBoolPref("browser.safebrowsing.malware.enabled", true);
prefBranch.setBoolPref("browser.safebrowsing.enabled", true);

Comment 2

[Philip Chee]

On the other hand we might have to implement the SafeBrowsing service in order for this to work. None of these test pages trigger anything in SeaMonkey do they?

http://www.mozilla.org/firefox/its-a-trap.html
http://www.mozilla.org/firefox/its-an-attack.html
(from http://www.mozilla.org/en-US/firefox/phishing-protection/)

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #1)
> > Would they miss to set browser.safebrowsing.malware.enabled preference?
> Possibly.

Confirmed by testing with Firefox.


(In reply to Philip Chee from comment #2)
> On the other hand we might have to implement the SafeBrowsing service in
> order for this to work.

If that were true, we should document it.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) test_classifier.html and test_classifier_worker.html: Set preference they depend on, Some rewrite and documentation [Checked in: Comment 8]

What about "XXX How is this part of the test supposed to work (= be checked)?" in classifierFrame.html ?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 610140 [details] [diff] [review]
(Av1) test_classifier.html and test_classifier_worker.html: Set preference they depend on, Some rewrite and documentation
[Checked in: Comment 8]

I'm sorry, but I don't know the safe-browsing code well enough to review this (I don't really know it at all in fact). Maybe someone from the network team knows this stuff?

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to Jonas Sicking (:sicking) from comment #5)

> I'm sorry, but I don't know the safe-browsing code well enough to review
> this (I don't really know it at all in fact).

I thought you would know it, as it looks like you r+/sr+ bug 441359 which added this code :-|

> Maybe someone from the network team knows this stuff?

Isn't Dave Camp, who wrote this code, around anymore?

Comment 7

[Dave Camp (:dcamp)]

r+'ed - :gcp (now cc'ed) is a capable safebrowsing reviewer too.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 610140 [details] [diff] [review]
(Av1) test_classifier.html and test_classifier_worker.html: Set preference they depend on, Some rewrite and documentation
[Checked in: Comment 8]

https://hg.mozilla.org/mozilla-central/rev/2c8a0c3b47f4


Dave, what about comment 4?

Comment 9

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1333486898.1333487878.6664.gz&fulltext=1
OS X 10.6 comm-central-trunk debug test mochitests-5/5 on 2012/04/03 14:01:38
{
9640 INFO TEST-PASS | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier.html | Should not load bad javascript - untouched should equal untouched
9641 INFO TEST-PASS | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier.html | Should not load bad css - visible should not equal hidden

9644 INFO TEST-PASS | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | blocked evilWorker.js - success should equal success
9645 INFO TEST-PASS | /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html | blocked importScripts('evilWorker.js') - success should equal success
}

V.Fixed

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 610140 [details] [diff] [review]
(Av1) test_classifier.html and test_classifier_worker.html: Set preference they depend on, Some rewrite and documentation
[Checked in: Comment 8]

Review of attachment 610140 [details] [diff] [review]:
-----------------------------------------------------------------

Regarding the XXX, isn't this the answer? 
// Make sure the css did not load.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #10)
> Regarding the XXX, isn't this the answer? 
> // Make sure the css did not load.

No, that code is related to evil.css, not import.css.