1272239 - Support completion for test database

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dimi Lee [:dimi]]

In Bug 1254766, we would like to trigger gethash request from a test database in testcase.

As suggested in Bug 1254766 Comment 32, we could only block gethash request for the lists in "disallow_completions" preference.

So in this bug we should allow list to trigger gethash as long as it is not in "disallow_completions".

Comment 1

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review commit: https://reviewboard.mozilla.org/r/52133/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52133/

Comment 2

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

Review commit: https://reviewboard.mozilla.org/r/52135/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52135/

Comment 3

[Dimi Lee [:dimi]]

This patch doesn't make "test-xxx" lists register gethash url. It just make it possible if we execute following steps in testcase:
1. add "test-xxx" to |browser.safebrowsing.provider.mozilla.lists| preference
2. remove "test-xxx" from |urlclassifier.disallow_completions| preference

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8751623 [details]
MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

obsolete because i would like to refine the testcase.

Comment 5

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review commit: https://reviewboard.mozilla.org/r/52706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52706/

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8751622 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52133/diff/1-2/

Comment 7

[Dimi Lee [:dimi]]

Comment on attachment 8751623 [details]
MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52135/diff/1-2/

Comment 8

[François Marier [:francois]]

Comment on attachment 8752687 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

https://reviewboard.mozilla.org/r/52706/#review50522

That's a great start!

I would suggest that we test two things here:

1. that resources get blocked when gethash returns successfully (i.e. what you have already done)
2. that resources are not blocked when gethash returns an error

The reason for that second one is that it will confirm that the test is valid and that the resources didn't just get blocked because of a 404, for example.

::: toolkit/components/url-classifier/tests/mochitest/gethashFrame.html:10
(Diff revision 1)
> +<script type="text/javascript">
> +
> +var scriptItem = "untouched";
> +
> +function checkLoads() {
> +  dump("[Dimi]Start...\n");

I think you forgot to remove that debug statement.

::: toolkit/components/url-classifier/tests/mochitest/gethashFrame.html:37
(Diff revision 1)
> +<script type="text/javascript" src="http://malware.example.com/tests/toolkit/components/url-classifier/tests/mochitest/evil.js"></script>
> +
> +<!-- Try loading from an uwanted software css URI -->
> +<link rel="stylesheet" type="text/css" href="http://unwanted.example.com/tests/toolkit/components/url-classifier/tests/mochitest/evil.css"></link>
> +
> +<!-- XXX How is this part of the test supposed to work (= be checked)? -->

That test also doesn't make any sense to me.

I think we could simply rewrite it to look like the other CSS one (maybe with a "styleCheck2" element) and add a second CSS file that will get loaded if the import succeeds.

Attempting to load a non-existent file (https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/toolkit/components/url-classifier/tests/mochitest/import.css) doesn't seem like a very good test to me.

Comment 9

[François Marier [:francois]]

Comment on attachment 8751623 [details]
MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

https://reviewboard.mozilla.org/r/52135/#review50544

Comment 10

[François Marier [:francois]]

Comment on attachment 8751622 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

https://reviewboard.mozilla.org/r/52133/#review50546

Comment 11

[Dimi Lee [:dimi]]

Comment on attachment 8751622 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52133/diff/2-3/

Comment 12

[Dimi Lee [:dimi]]

Comment on attachment 8751623 [details]
MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52135/diff/2-3/

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8752687 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52706/diff/1-2/

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 8751622 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52133/diff/3-4/

Comment 15

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1272239 - P1. Support completion for test database. r=francois

Review commit: https://reviewboard.mozilla.org/r/56218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56218/

Comment 16

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois

Review commit: https://reviewboard.mozilla.org/r/56220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56220/

Comment 17

[Dimi Lee [:dimi]]

Attachment: Bug 1272239 - P1. Support completion for test database.

Comment 18

[Dimi Lee [:dimi]]

Attachment: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager

Comment 19

[Dimi Lee [:dimi]]

Attachment: Bug 1272239 - P3. Testcase, test gethash.

Sorry I guess I totally screw up the mozreview after trying to just update one changeset...

Use the old way instead...

Comment 20

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #8)
> 
> That test also doesn't make any sense to me.
> 
> I think we could simply rewrite it to look like the other CSS one (maybe
> with a "styleCheck2" element) and add a second CSS file that will get loaded
> if the import succeeds.
> 
> Attempting to load a non-existent file
> (https://dxr.mozilla.org/mozilla-central/rev/
> c4449eab07d39e20ea315603f1b1863eeed7dcfe/toolkit/components/url-classifier/
> tests/mochitest/import.css) doesn't seem like a very good test to me.

I am not sure if I understand here correctly, but from comment i guess what we are trying to verify is to make sure "import" a bad css from a good css should be blocked.

so i add two style checks in this patch:
1. test the good css(import.css) should be loaded.
2. test the bad css(bad.css) imported by good css should not be loaded.

please correct me if i am wrong here.

Comment 21

[François Marier [:francois]]

Comment on attachment 8757824 [details] [diff] [review]
Bug 1272239 - P3. Testcase, test gethash.

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #20)
> I am not sure if I understand here correctly, but from comment i guess what
> we are trying to verify is to make sure "import" a bad css from a good css
> should be blocked.
> 
> so i add two style checks in this patch:
> 1. test the good css(import.css) should be loaded.
> 2. test the bad css(bad.css) imported by good css should not be loaded.

Thanks, the new tests are what I was looking for.

Comment 22

[Dimi Lee [:dimi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9111ecec1c91

Comment 23

[Dimi Lee [:dimi]]

Attachment: P1. Support completion for test database.

rebase to latest code

Comment 24

[Dimi Lee [:dimi]]

Attachment: P2. Testcase, only tables with provider could register gethash url in listmanager.

Comment 25

[Dimi Lee [:dimi]]

Attachment: P3. Testcase, test gethash.

Comment 26

[Dimi Lee [:dimi]]

try with rebased code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08032b6c97d9

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc114bc5a88e
P1. Support completion for test database. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0b3881c1c4
P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/5042da9fc11c
P3. Testcase, test gethash. r=francois

Comment 28

[Carsten Book [:Tomcat]]

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=30197260&repo=mozilla-inbound

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/672fff99c00a
Backed out changeset 5042da9fc11c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04c12e6bf35
Backed out changeset ed0b3881c1c4 
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b498a9a36d
Backed out changeset dc114bc5a88e for test failures in own test

Comment 30

[Dimi Lee [:dimi]]

The testcase that fail want to make sure every table with a provider should have a gethash/update url.
But currently change tables in preferences do not really re-register gethash/update url.

In this case "urlclassifier.trackingTable" is initially set to "test-track-simple" so only this table will register gethash/update url. But after preference reset to "mozstd-track-digest256" it won't trigger any re-register behavior so gethash url won't exist for "mozstd-track-digest256", then the testcase fail.

I will fix this in Bug 1281083

Comment 31

[Dimi Lee [:dimi]]

Attachment: P3. Testcase, test gethash

Missing testcase in mochitest.ini

Comment 32

[Dimi Lee [:dimi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=531ccce9b4cc

Comment 33

[Dimi Lee [:dimi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d334c6a9cfd

Comment 34

[Dimi Lee [:dimi]]

Attachment: (r+d) P1. Support completion for test database.

Rebase to latest code

Comment 35

[Dimi Lee [:dimi]]

Attachment: (r+d) P2. Testcase, only tables with provider could register gethash url in listmanager.

Comment 36

[Dimi Lee [:dimi]]

Attachment: (r+d) P3. Testcase, test gethash.

Comment 37

[Dimi Lee [:dimi]]

try result is in Comment 33

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74de16c8bed7
Part 1: Support completion for test database. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe1ca72b7d7
Part 2: Testcase, only tables with provider could register gethash url in listmanager. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ea967f04c8
Part 3: Testcase, test gethash. r=francois

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74de16c8bed7
https://hg.mozilla.org/mozilla-central/rev/cfe1ca72b7d7
https://hg.mozilla.org/mozilla-central/rev/59ea967f04c8