Closed Bug 1264169 Opened 6 years ago Closed 6 years ago

test_classifier.html doesn't remove url added to malware database when finish

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

Details

Attachments

(2 files, 1 obsolete file)

In test_classifier.html, we will add malware.example.com to test-malware-simple db and unwanted.example.com to test-unwanted-simple db.
But after the testcase is finished, we didn't remove it from database, so if there is any other testcase run after test_classifier.html with safebrowing on, those urls will be treated as invalid url.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Depends on: 1262406
Depends on: 1262339
Blocks: 1262339
No longer depends on: 1262339
Blocks: 1262406
No longer depends on: 1262406
Depends on: 1264829
Attached patch WIP patch (obsolete) — Splinter Review
still working on figuring out what is the correct sub chunk format when update db
Attachment #8742882 - Attachment is obsolete: true
Comment on attachment 8743106 [details]
MozReview Request: Bug 1264169 - P1. test_classifier.html doesn't remove url added to malware database when finish. r=francois

https://reviewboard.mozilla.org/r/47567/#review44719

Nice work!

::: toolkit/components/url-classifier/tests/mochitest/classifierHelper.js:14
(Diff revision 1)
> +// the callback follow correct order.
> +classifierHelper._updates = [];
> +
> +// Keep urls added to database, those urls should be automatically
> +// removed after test complete.
> +classifierHelper._updateData = [];

nit: "_updatesToCleanup" would make the code a little bit clearer

::: toolkit/components/url-classifier/tests/mochitest/classifierHelper.js:38
(Diff revision 1)
> +classifierHelper.removeUrlFromDB = function(updateData, onsuccess, onerror) {
> +  var testUpdate = "";
> +  for (var update of updateData) {
> +    testUpdate +=
> +      "n:1000\ni:" + update.db + "\nsd:1\n" +
> +      "s:524:32:" + (4 + update.url.length) + "\n" +

That's a really weird format. I assume you found something that works.

It will be great when we are rid of the \*-simple lists.
Attachment #8743106 - Flags: review?(francois) → review+
Review commit: https://reviewboard.mozilla.org/r/48417/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48417/
Attachment #8743106 - Attachment description: MozReview Request: Bug 1264169 - test_classifier.html doesn't remove url added to malware database when finish. r?francois → MozReview Request: Bug 1264169 - P1. test_classifier.html doesn't remove url added to malware database when finish. r=francois
Attachment #8744235 - Flags: review?(francois)
Comment on attachment 8743106 [details]
MozReview Request: Bug 1264169 - P1. test_classifier.html doesn't remove url added to malware database when finish. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47567/diff/1-2/
(In reply to François Marier [:francois] from comment #3)
> 
> That's a really weird format. I assume you found something that works.
> 
> It will be great when we are rid of the \*-simple lists.

I upload another patch to see if this can make things more clear.
(In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> (In reply to François Marier [:francois] from comment #3)
> > 
> > That's a really weird format. I assume you found something that works.
> > 
> > It will be great when we are rid of the \*-simple lists.
> 
> I upload another patch to see if this can make things more clear.

Sorry, I didn't mean to imply you had to fix this. If it works, it's fine. I'm just surprised by what it looks like :)
Attachment #8744235 - Flags: review?(francois) → review+
Comment on attachment 8744235 [details]
MozReview Request: Bug 1264169 - P2. Refine classifierHelper chunk format. r?francois

https://reviewboard.mozilla.org/r/48417/#review45269

::: toolkit/components/url-classifier/tests/mochitest/classifierHelper.js:8
(Diff revision 1)
>  }
>  
>  const CLASSIFIER_COMMON_URL = SimpleTest.getTestFileURL("classifierCommon.js");
>  var classifierCommonScript = SpecialPowers.loadChromeScript(CLASSIFIER_COMMON_URL);
>  
> +const ADD_CHUNKNUM = 524;

Nit: you could also just have "CHUNKNUM" here since we are adding and removing the same chunk (that's the whole point of your patch, we need to cleanup the test entries).
(In reply to François Marier [:francois] from comment #7)
> Sorry, I didn't mean to imply you had to fix this. If it works, it's fine.
> I'm just surprised by what it looks like :)

Not at all, I think the code could be refined to make it more clear, so i re-write that part :)
(In reply to François Marier [:francois] from comment #8)
> Nit: you could also just have "CHUNKNUM" here since we are adding and
> removing the same chunk (that's the whole point of your patch, we need to
> cleanup the test entries).

Oh sorry, I think I may make it confusing by setting ADD_CHUNKNUM and SUB_CHUNKNUM to the same value, actually they don't have to be the same.
The way here to clear add chunk is by specifying the ADD_CHUNKNUM in CHUNKDATA of SUB CHUNK(defined in spec):
var CHUNKDATA = ADD_CHUNKNUM + ":" + update.url;

I will modify the patch by making it different to avoid misunderstanding.
Comment on attachment 8744235 [details]
MozReview Request: Bug 1264169 - P2. Refine classifierHelper chunk format. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48417/diff/1-2/
(In reply to Dimi Lee[:dimi][:dlee] from comment #10)
> Oh sorry, I think I may make it confusing by setting ADD_CHUNKNUM and
> SUB_CHUNKNUM to the same value, actually they don't have to be the same.
> The way here to clear add chunk is by specifying the ADD_CHUNKNUM in
> CHUNKDATA of SUB CHUNK(defined in spec):
> var CHUNKDATA = ADD_CHUNKNUM + ":" + update.url;

You're right, I got confused. Thanks for making it clearer!
https://hg.mozilla.org/mozilla-central/rev/941cedab6a7f
https://hg.mozilla.org/mozilla-central/rev/15655faddab7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.