Closed
Bug 1272239
Opened 8 years ago
Closed 8 years ago
Support completion for test database
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: dlee, Assigned: dlee)
References
Details
(Whiteboard: #sbv4-m0)
Attachments
(3 files, 12 obsolete files)
2.53 KB,
patch
|
dlee
:
review+
|
Details | Diff | Splinter Review |
3.97 KB,
patch
|
dlee
:
review+
|
Details | Diff | Splinter Review |
19.08 KB,
patch
|
dlee
:
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".
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52133/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52133/
Attachment #8751622 -
Flags: review?(francois)
Attachment #8751623 -
Flags: review?(francois)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52135/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52135/
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8751622 -
Attachment is obsolete: true
Attachment #8751622 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Summary: Only block completion for lists in "disallow_completions" preference → Support completion for test database
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
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 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8751623 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8752687 -
Attachment is obsolete: true
Attachment #8752687 -
Flags: review?(francois)
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/56218/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56218/
Attachment #8757815 -
Flags: review?(francois)
Attachment #8757816 -
Flags: review?(francois)
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/56220/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56220/
Assignee | ||
Updated•8 years ago
|
Attachment #8751622 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8757815 -
Attachment is obsolete: true
Attachment #8757815 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8757816 -
Attachment is obsolete: true
Attachment #8757816 -
Flags: review?(francois)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8757821 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8757823 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
(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•8 years ago
|
||
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+
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9111ecec1c91
Assignee | ||
Comment 23•8 years ago
|
||
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+
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8762885 -
Flags: review+
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8762886 -
Flags: review+
Assignee | ||
Comment 26•8 years ago
|
||
try with rebased code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08032b6c97d9
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 27•8 years ago
|
||
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
Comment 28•8 years ago
|
||
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=30197260&repo=mozilla-inbound
Flags: needinfo?(dlee)
Comment 29•8 years ago
|
||
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
Assignee | ||
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
Missing testcase in mochitest.ini
Attachment #8762886 -
Attachment is obsolete: true
Attachment #8764801 -
Flags: review+
Updated•8 years ago
|
Whiteboard: #sbv4-m0
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=531ccce9b4cc
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d334c6a9cfd
Assignee | ||
Comment 34•8 years ago
|
||
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+
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8773555 -
Flags: review+
Assignee | ||
Comment 36•8 years ago
|
||
Attachment #8773557 -
Flags: review+
Comment 38•8 years ago
|
||
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
Comment 39•8 years ago
|
||
bugherder |
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: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•