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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dimi Lee [ooo 16.04 - 21.04］]

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.

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WIP patch

still working on figuring out what is the correct sub chunk format when update db

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: MozReview Request: Bug 1264169 - P1. test_classifier.html doesn't remove url added to malware database when finish. r=francois

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

Comment 3

[François Marier [: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

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.

Comment 4

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: MozReview Request: Bug 1264169 - P2. Refine classifierHelper chunk format. r?francois

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

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

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/

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

(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.

Comment 7

[François Marier [:francois]]

(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 :)

Comment 8

[François Marier [:francois]]

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).

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

(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 :)

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

(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 11

[Dimi Lee [ooo 16.04 - 21.04］]

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/

Comment 12

[François Marier [:francois]]

(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!

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae12730c06b5

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/941cedab6a7f
https://hg.mozilla.org/integration/fx-team/rev/15655faddab7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/941cedab6a7f
https://hg.mozilla.org/mozilla-central/rev/15655faddab7