Closed Bug 1272239 Opened 4 years ago Closed 3 years ago

Support completion for test database

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(3 files, 12 obsolete files)

2.53 KB, patch
dimi
: review+
Details | Diff | Splinter Review
3.97 KB, patch
dimi
: review+
Details | Diff | Splinter Review
19.08 KB, patch
dimi
: review+
Details | Diff | Splinter Review
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".
Blocks: 1254766
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 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.
Attachment #8751623 - Attachment is obsolete: true
Attachment #8751623 - Flags: review?(francois)
Attachment #8751622 - Attachment is obsolete: true
Attachment #8751622 - Flags: review?(francois)
Summary: Only block completion for lists in "disallow_completions" preference → Support completion for test database
Review commit: https://reviewboard.mozilla.org/r/52706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52706/
Attachment #8751622 - Attachment description: MozReview Request: Bug 1272239 - Only block completion for lists in disallow_completions preference. r?francois → MozReview Request: Bug 1272239 - P1. Support completion for test database. r?francois
Attachment #8751623 - Attachment description: MozReview Request: Bug 1272239 - Testcase. r?francois → MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r?francois
Attachment #8751622 - Attachment is obsolete: false
Attachment #8751623 - Attachment is obsolete: false
Attachment #8752687 - Flags: review?(francois)
Attachment #8751622 - Flags: review?(francois)
Attachment #8751623 - Flags: review?(francois)
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 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/
Depends on: 1274105
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.
Attachment #8752687 - Flags: review?(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
Attachment #8751623 - Flags: review?(francois) → review+
Comment on attachment 8751622 [details]
MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois

https://reviewboard.mozilla.org/r/52133/#review50546
Attachment #8751622 - Flags: review?(francois) → review+
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/
Attachment #8751622 - Attachment description: MozReview Request: Bug 1272239 - P1. Support completion for test database. r?francois → MozReview Request: Bug 1272239 - P1. Support completion for test database. r=francois
Attachment #8751623 - Attachment description: MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r?francois → MozReview Request: Bug 1272239 - P2. Testcase, only tables with provider could register gethash url in listmanager. r=francois
Attachment #8752687 - Flags: review?(francois)
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 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 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/
Attachment #8751622 - Attachment description: MozReview Request: Bug 1272239 - P1. Support completion for test database. r=francois → MozReview Request: Bug 1272239 - P3. Testcase, test gethash. r?francois
Attachment #8751623 - Attachment is obsolete: true
Attachment #8752687 - Attachment is obsolete: true
Attachment #8752687 - Flags: review?(francois)
Attachment #8751622 - Attachment is obsolete: true
Attachment #8757815 - Attachment is obsolete: true
Attachment #8757815 - Flags: review?(francois)
Attachment #8757816 - Attachment is obsolete: true
Attachment #8757816 - Flags: review?(francois)
Sorry I guess I totally screw up the mozreview after trying to just update one changeset...

Use the old way instead...
Attachment #8757824 - Flags: review?(francois)
(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 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.
Attachment #8757824 - Flags: review?(francois) → review+
rebase to latest code
Attachment #8757821 - Attachment is obsolete: true
Attachment #8757823 - Attachment is obsolete: true
Attachment #8757824 - Attachment is obsolete: true
Attachment #8762884 - Flags: review+
Attached patch P3. Testcase, test gethash. (obsolete) — Splinter Review
Attachment #8762886 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=30197260&repo=mozilla-inbound
Flags: needinfo?(dlee)
Backout by cbook@mozilla.com:
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
Depends on: 1281083
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
No longer depends on: 1281083
Flags: needinfo?(dlee)
Depends on: 1281083
Attached patch P3. Testcase, test gethash (obsolete) — Splinter Review
Missing testcase in mochitest.ini
Attachment #8762886 - Attachment is obsolete: true
Attachment #8764801 - Flags: review+
Whiteboard: #sbv4-m0
Rebase to latest code
Attachment #8762884 - Attachment is obsolete: true
Attachment #8762885 - Attachment is obsolete: true
Attachment #8764801 - Attachment is obsolete: true
Attachment #8773554 - Flags: review+
try result is in Comment 33
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74de16c8bed7
https://hg.mozilla.org/mozilla-central/rev/cfe1ca72b7d7
https://hg.mozilla.org/mozilla-central/rev/59ea967f04c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.